[PATCH] drm: Allow modeset on unregisted connectors unconditionally

[PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

We allowed modesetting an unregistered connector only in the case the
mode is getting disabled on the connector.

The reason for this check was the lack of proper refcounting for the
backing memory objects. That problem has been solved meanwhile so there
is no reason any more to reject the modesetting in general. The check
for that also makes driver internal modesets more cumbersome where we
need to add exemptions for the cases where we do need to allow the
modeset even for unregistered connectors. One such case is the
restoration of the mode during resume.

Simplify things by removing the unneeded check. I can't see how
modesetting an unregistered connector can cause any problem and the race
(described in the code comment) can anyway result in such a modeset (if
the connector is unregistered right after the check).

Cc: Lyude Paul <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2e0cb4246cbd..e94e69483498 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	crtc_state = drm_atomic_get_new_crtc_state(state,
-						   new_connector_state->crtc);
-	/*
-	 * For compatibility with legacy users, we want to make sure that
-	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
-	 * which would result in anything else must be considered invalid, to
-	 * avoid turning on new displays on dead connectors.
-	 *
-	 * Since the connector can be unregistered at any point during an
-	 * atomic check or commit, this is racy. But that's OK: all we care
-	 * about is ensuring that userspace can't do anything but shut off the
-	 * display on a connector that was destroyed after it's been notified,
-	 * not before.
-	 *
-	 * Additionally, we also want to ignore connector registration when
-	 * we're trying to restore an atomic state during system resume since
-	 * there's a chance the connector may have been destroyed during the
-	 * process, but it's better to ignore that then cause
-	 * drm_atomic_helper_resume() to fail.
-	 */
-	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
-	    crtc_state->active) {
-		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
-				 connector->base.id, connector->name);
-		return -EINVAL;
-	}
-
 	funcs = connector->helper_private;
 
 	if (funcs->atomic_best_encoder)
@@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	set_best_encoder(state, new_connector_state, new_encoder);
 
+	crtc_state = drm_atomic_get_new_crtc_state(state,
+						   new_connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Daniel Vetter]

On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> We allowed modesetting an unregistered connector only in the case the
> mode is getting disabled on the connector.
> 
> The reason for this check was the lack of proper refcounting for the
> backing memory objects. That problem has been solved meanwhile so there
> is no reason any more to reject the modesetting in general.

I'm not parsing this at all ... maybe references to the commits that fix
this? Or do you mean the refcounting work for all the things hanging of
connectors, including the entire mst tree?

> The check
> for that also makes driver internal modesets more cumbersome where we
> need to add exemptions for the cases where we do need to allow the
> modeset even for unregistered connectors. One such case is the
> restoration of the mode during resume.

Yeah this one actually makes sense to me. We could still keep this check
here, but for the atomic ioctl only when called from userspace. But iirc
Lyude also said she has some plans here, so no idea whether that all fits.

> Simplify things by removing the unneeded check. I can't see how
> modesetting an unregistered connector can cause any problem and the race
> (described in the code comment) can anyway result in such a modeset (if
> the connector is unregistered right after the check).

Not saying we don't need this, but there's fairly enormous amounts of
history behind all this stuff, and lots of discussions. Would be good to
at least reference those, so we have a good story for when this then all
goes wrong again.
-Daniel

> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2e0cb4246cbd..e94e69483498 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(state,
> -						   new_connector_state->crtc);
> -	/*
> -	 * For compatibility with legacy users, we want to make sure that
> -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> -	 * which would result in anything else must be considered invalid, to
> -	 * avoid turning on new displays on dead connectors.
> -	 *
> -	 * Since the connector can be unregistered at any point during an
> -	 * atomic check or commit, this is racy. But that's OK: all we care
> -	 * about is ensuring that userspace can't do anything but shut off the
> -	 * display on a connector that was destroyed after it's been notified,
> -	 * not before.
> -	 *
> -	 * Additionally, we also want to ignore connector registration when
> -	 * we're trying to restore an atomic state during system resume since
> -	 * there's a chance the connector may have been destroyed during the
> -	 * process, but it's better to ignore that then cause
> -	 * drm_atomic_helper_resume() to fail.
> -	 */
> -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> -	    crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> -				 connector->base.id, connector->name);
> -		return -EINVAL;
> -	}
> -
>  	funcs = connector->helper_private;
>  
>  	if (funcs->atomic_best_encoder)
> @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	set_best_encoder(state, new_connector_state, new_encoder);
>  
> +	crtc_state = drm_atomic_get_new_crtc_state(state,
> +						   new_connector_state->crtc);
>  	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > We allowed modesetting an unregistered connector only in the case the
> > mode is getting disabled on the connector.
> > 
> > The reason for this check was the lack of proper refcounting for the
> > backing memory objects. That problem has been solved meanwhile so there
> > is no reason any more to reject the modesetting in general.
> 
> I'm not parsing this at all ... maybe references to the commits that fix
> this? Or do you mean the refcounting work for all the things hanging of
> connectors, including the entire mst tree?

Yes the check was added to solve the issue related to the removal of MST
connectors that could happen asynchronously wrt. a modeset referring to
that MST connector.  That could happen since the MST core doesn't hold
any locks (for instance the connection_mutex) during removing an MST
connector that would prevent doing a modeset at the same time.

Adding the refcounting for such MST connectors (via the
drm_connector_get()/drm_connector_put()) got rid of the above problem.

> 
> > The check
> > for that also makes driver internal modesets more cumbersome where we
> > need to add exemptions for the cases where we do need to allow the
> > modeset even for unregistered connectors. One such case is the
> > restoration of the mode during resume.
> 
> Yeah this one actually makes sense to me. We could still keep this check
> here, but for the atomic ioctl only when called from userspace. But iirc
> Lyude also said she has some plans here, so no idea whether that all fits.
> 
> > Simplify things by removing the unneeded check. I can't see how
> > modesetting an unregistered connector can cause any problem and the race
> > (described in the code comment) can anyway result in such a modeset (if
> > the connector is unregistered right after the check).
> 
> Not saying we don't need this, but there's fairly enormous amounts of
> history behind all this stuff, and lots of discussions. Would be good to
> at least reference those, so we have a good story for when this then all
> goes wrong again.

I still don't see why this check is needed. There is no justification
for it - besides the original reason for it as discussed above about the
refcounting problem, which is solved now - so I think we should remove
it, instead of just making it a special case for the user space modeset.

As I wrote a user space modeset can end up anyway doing a modeset on an
unregistered connector when the unregistering - by MST core - happens just
right after the check.

> -Daniel
> 
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> >  1 file changed, 2 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 2e0cb4246cbd..e94e69483498 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> >  		return 0;
> >  	}
> >  
> > -	crtc_state = drm_atomic_get_new_crtc_state(state,
> > -						   new_connector_state->crtc);
> > -	/*
> > -	 * For compatibility with legacy users, we want to make sure that
> > -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > -	 * which would result in anything else must be considered invalid, to
> > -	 * avoid turning on new displays on dead connectors.
> > -	 *
> > -	 * Since the connector can be unregistered at any point during an
> > -	 * atomic check or commit, this is racy. But that's OK: all we care
> > -	 * about is ensuring that userspace can't do anything but shut off the
> > -	 * display on a connector that was destroyed after it's been notified,
> > -	 * not before.
> > -	 *
> > -	 * Additionally, we also want to ignore connector registration when
> > -	 * we're trying to restore an atomic state during system resume since
> > -	 * there's a chance the connector may have been destroyed during the
> > -	 * process, but it's better to ignore that then cause
> > -	 * drm_atomic_helper_resume() to fail.
> > -	 */
> > -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > -	    crtc_state->active) {
> > -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > -				 connector->base.id, connector->name);
> > -		return -EINVAL;
> > -	}
> > -
> >  	funcs = connector->helper_private;
> >  
> >  	if (funcs->atomic_best_encoder)
> > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> >  
> >  	set_best_encoder(state, new_connector_state, new_encoder);
> >  
> > +	crtc_state = drm_atomic_get_new_crtc_state(state,
> > +						   new_connector_state->crtc);
> >  	crtc_state->connectors_changed = true;
> >  
> >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Daniel Vetter]

On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > We allowed modesetting an unregistered connector only in the case the
> > > mode is getting disabled on the connector.
> > > 
> > > The reason for this check was the lack of proper refcounting for the
> > > backing memory objects. That problem has been solved meanwhile so there
> > > is no reason any more to reject the modesetting in general.
> > 
> > I'm not parsing this at all ... maybe references to the commits that fix
> > this? Or do you mean the refcounting work for all the things hanging of
> > connectors, including the entire mst tree?
> 
> Yes the check was added to solve the issue related to the removal of MST
> connectors that could happen asynchronously wrt. a modeset referring to
> that MST connector.  That could happen since the MST core doesn't hold
> any locks (for instance the connection_mutex) during removing an MST
> connector that would prevent doing a modeset at the same time.
> 
> Adding the refcounting for such MST connectors (via the
> drm_connector_get()/drm_connector_put()) got rid of the above problem.

We added the check way after that stuff landed. Before all the connector
reworking connectors were forcefully disabled by the kernel. The idea
behind this check is to make sure that that userspace notices a connector
is gone (only thing that's not allowed is enabling it, you can keep
pageflipping). I think we've always had behaviour like ever since mst (all
userspace has some "oops mst connector probably gone" failure catching
around modesets).

So no idea what all blows up if we stop catching userspace this way.

Now very much possible I'm getting all this wrong again or missing
something, this stuff is often way over my head. But I'm really vary of
breaking userspace here. E.g. just the drm_connector_get/put lifetime
changes results in some userspace breaking if you unplug/replug fast
enough, because then it doesn't notice the connector change anymore. I
couldn't figure out a way to paper over that regression without
reintroduce the rather broken and oops-prone old connector lifetime
management.

> > > The check
> > > for that also makes driver internal modesets more cumbersome where we
> > > need to add exemptions for the cases where we do need to allow the
> > > modeset even for unregistered connectors. One such case is the
> > > restoration of the mode during resume.
> > 
> > Yeah this one actually makes sense to me. We could still keep this check
> > here, but for the atomic ioctl only when called from userspace. But iirc
> > Lyude also said she has some plans here, so no idea whether that all fits.
> > 
> > > Simplify things by removing the unneeded check. I can't see how
> > > modesetting an unregistered connector can cause any problem and the race
> > > (described in the code comment) can anyway result in such a modeset (if
> > > the connector is unregistered right after the check).
> > 
> > Not saying we don't need this, but there's fairly enormous amounts of
> > history behind all this stuff, and lots of discussions. Would be good to
> > at least reference those, so we have a good story for when this then all
> > goes wrong again.
> 
> I still don't see why this check is needed. There is no justification
> for it - besides the original reason for it as discussed above about the
> refcounting problem, which is solved now - so I think we should remove
> it, instead of just making it a special case for the user space modeset.
> 
> As I wrote a user space modeset can end up anyway doing a modeset on an
> unregistered connector when the unregistering - by MST core - happens just
> right after the check.

Yup. Always been like that.
-Daniel

> 
> > -Daniel
> > 
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 2e0cb4246cbd..e94e69483498 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > >  		return 0;
> > >  	}
> > >  
> > > -	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > -						   new_connector_state->crtc);
> > > -	/*
> > > -	 * For compatibility with legacy users, we want to make sure that
> > > -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > -	 * which would result in anything else must be considered invalid, to
> > > -	 * avoid turning on new displays on dead connectors.
> > > -	 *
> > > -	 * Since the connector can be unregistered at any point during an
> > > -	 * atomic check or commit, this is racy. But that's OK: all we care
> > > -	 * about is ensuring that userspace can't do anything but shut off the
> > > -	 * display on a connector that was destroyed after it's been notified,
> > > -	 * not before.
> > > -	 *
> > > -	 * Additionally, we also want to ignore connector registration when
> > > -	 * we're trying to restore an atomic state during system resume since
> > > -	 * there's a chance the connector may have been destroyed during the
> > > -	 * process, but it's better to ignore that then cause
> > > -	 * drm_atomic_helper_resume() to fail.
> > > -	 */
> > > -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > -	    crtc_state->active) {
> > > -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > -				 connector->base.id, connector->name);
> > > -		return -EINVAL;
> > > -	}
> > > -
> > >  	funcs = connector->helper_private;
> > >  
> > >  	if (funcs->atomic_best_encoder)
> > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > >  
> > >  	set_best_encoder(state, new_connector_state, new_encoder);
> > >  
> > > +	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > +						   new_connector_state->crtc);
> > >  	crtc_state->connectors_changed = true;
> > >  
> > >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > -- 
> > > 2.17.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

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

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

On Mon, May 20, 2019 at 09:23:00PM +0200, Daniel Vetter wrote:
> On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> > On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > > We allowed modesetting an unregistered connector only in the case the
> > > > mode is getting disabled on the connector.
> > > > 
> > > > The reason for this check was the lack of proper refcounting for the
> > > > backing memory objects. That problem has been solved meanwhile so there
> > > > is no reason any more to reject the modesetting in general.
> > > 
> > > I'm not parsing this at all ... maybe references to the commits that fix
> > > this? Or do you mean the refcounting work for all the things hanging of
> > > connectors, including the entire mst tree?
> > 
> > Yes the check was added to solve the issue related to the removal of MST
> > connectors that could happen asynchronously wrt. a modeset referring to
> > that MST connector.  That could happen since the MST core doesn't hold
> > any locks (for instance the connection_mutex) during removing an MST
> > connector that would prevent doing a modeset at the same time.
> > 
> > Adding the refcounting for such MST connectors (via the
> > drm_connector_get()/drm_connector_put()) got rid of the above problem.
> 
> We added the check way after that stuff landed. Before all the connector
> reworking connectors were forcefully disabled by the kernel. The idea
> behind this check is to make sure that that userspace notices a connector
> is gone (only thing that's not allowed is enabling it, you can keep
> pageflipping). I think we've always had behaviour like ever since mst (all
> userspace has some "oops mst connector probably gone" failure catching
> around modesets).

Right, pageflipping works.

> So no idea what all blows up if we stop catching userspace this way.
> 
> Now very much possible I'm getting all this wrong again or missing
> something, this stuff is often way over my head. But I'm really vary of
> breaking userspace here. E.g. just the drm_connector_get/put lifetime
> changes results in some userspace breaking if you unplug/replug fast
> enough, because then it doesn't notice the connector change anymore. I
> couldn't figure out a way to paper over that regression without
> reintroduce the rather broken and oops-prone old connector lifetime
> management.

Yes, but what is the the actual description of the failing scenario? I
can't see how anything can go wrong without this check. The resume time
restoration modeset may have to act in the same way on an old connector.

I don't understand how userspace would not notice the connector change.
It will get a hotplug uevent in response to which it would have to do a
detect which returns to it the updated information about the new MST
connector tree.

> > > > The check
> > > > for that also makes driver internal modesets more cumbersome where we
> > > > need to add exemptions for the cases where we do need to allow the
> > > > modeset even for unregistered connectors. One such case is the
> > > > restoration of the mode during resume.
> > > 
> > > Yeah this one actually makes sense to me. We could still keep this check
> > > here, but for the atomic ioctl only when called from userspace. But iirc
> > > Lyude also said she has some plans here, so no idea whether that all fits.
> > > 
> > > > Simplify things by removing the unneeded check. I can't see how
> > > > modesetting an unregistered connector can cause any problem and the race
> > > > (described in the code comment) can anyway result in such a modeset (if
> > > > the connector is unregistered right after the check).
> > > 
> > > Not saying we don't need this, but there's fairly enormous amounts of
> > > history behind all this stuff, and lots of discussions. Would be good to
> > > at least reference those, so we have a good story for when this then all
> > > goes wrong again.
> > 
> > I still don't see why this check is needed. There is no justification
> > for it - besides the original reason for it as discussed above about the
> > refcounting problem, which is solved now - so I think we should remove
> > it, instead of just making it a special case for the user space modeset.
> > 
> > As I wrote a user space modeset can end up anyway doing a modeset on an
> > unregistered connector when the unregistering - by MST core - happens just
> > right after the check.
> 
> Yup. Always been like that.
> -Daniel
> 
> > 
> > > -Daniel
> > > 
> > > > 
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 2e0cb4246cbd..e94e69483498 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > -	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > -						   new_connector_state->crtc);
> > > > -	/*
> > > > -	 * For compatibility with legacy users, we want to make sure that
> > > > -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > > -	 * which would result in anything else must be considered invalid, to
> > > > -	 * avoid turning on new displays on dead connectors.
> > > > -	 *
> > > > -	 * Since the connector can be unregistered at any point during an
> > > > -	 * atomic check or commit, this is racy. But that's OK: all we care
> > > > -	 * about is ensuring that userspace can't do anything but shut off the
> > > > -	 * display on a connector that was destroyed after it's been notified,
> > > > -	 * not before.
> > > > -	 *
> > > > -	 * Additionally, we also want to ignore connector registration when
> > > > -	 * we're trying to restore an atomic state during system resume since
> > > > -	 * there's a chance the connector may have been destroyed during the
> > > > -	 * process, but it's better to ignore that then cause
> > > > -	 * drm_atomic_helper_resume() to fail.
> > > > -	 */
> > > > -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > > -	    crtc_state->active) {
> > > > -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > > -				 connector->base.id, connector->name);
> > > > -		return -EINVAL;
> > > > -	}
> > > > -
> > > >  	funcs = connector->helper_private;
> > > >  
> > > >  	if (funcs->atomic_best_encoder)
> > > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > > >  
> > > >  	set_best_encoder(state, new_connector_state, new_encoder);
> > > >  
> > > > +	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > +						   new_connector_state->crtc);
> > > >  	crtc_state->connectors_changed = true;
> > > >  
> > > >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Daniel Vetter]

On Mon, May 20, 2019 at 10:06 PM Imre Deak <imre.deak@intel.com> wrote:
> On Mon, May 20, 2019 at 09:23:00PM +0200, Daniel Vetter wrote:
> > On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> > > On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > > > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > > > We allowed modesetting an unregistered connector only in the case the
> > > > > mode is getting disabled on the connector.
> > > > >
> > > > > The reason for this check was the lack of proper refcounting for the
> > > > > backing memory objects. That problem has been solved meanwhile so there
> > > > > is no reason any more to reject the modesetting in general.
> > > >
> > > > I'm not parsing this at all ... maybe references to the commits that fix
> > > > this? Or do you mean the refcounting work for all the things hanging of
> > > > connectors, including the entire mst tree?
> > >
> > > Yes the check was added to solve the issue related to the removal of MST
> > > connectors that could happen asynchronously wrt. a modeset referring to
> > > that MST connector.  That could happen since the MST core doesn't hold
> > > any locks (for instance the connection_mutex) during removing an MST
> > > connector that would prevent doing a modeset at the same time.
> > >
> > > Adding the refcounting for such MST connectors (via the
> > > drm_connector_get()/drm_connector_put()) got rid of the above problem.
> >
> > We added the check way after that stuff landed. Before all the connector
> > reworking connectors were forcefully disabled by the kernel. The idea
> > behind this check is to make sure that that userspace notices a connector
> > is gone (only thing that's not allowed is enabling it, you can keep
> > pageflipping). I think we've always had behaviour like ever since mst (all
> > userspace has some "oops mst connector probably gone" failure catching
> > around modesets).
>
> Right, pageflipping works.
>
> > So no idea what all blows up if we stop catching userspace this way.
> >
> > Now very much possible I'm getting all this wrong again or missing
> > something, this stuff is often way over my head. But I'm really vary of
> > breaking userspace here. E.g. just the drm_connector_get/put lifetime
> > changes results in some userspace breaking if you unplug/replug fast
> > enough, because then it doesn't notice the connector change anymore. I
> > couldn't figure out a way to paper over that regression without
> > reintroduce the rather broken and oops-prone old connector lifetime
> > management.
>
> Yes, but what is the the actual description of the failing scenario? I
> can't see how anything can go wrong without this check. The resume time
> restoration modeset may have to act in the same way on an old connector.

That's what the !state->duplicated thing is meant to check for btw.

> I don't understand how userspace would not notice the connector change.
> It will get a hotplug uevent in response to which it would have to do a
> detect which returns to it the updated information about the new MST
> connector tree.

uevent handling can take positively forever, at which point the new
connector could already be plugged in, and then userspace makes a mess
aliasing the two since the path property matches. Just needs a wobbly
cable. This is the issue with the "fixed" lifetime management, and I'm
wary of breaking more stuff.

Other way round: What do we gain if userspace is allowed to turn on a
connector again which doesn't exist and will not ever show any pixels?
I'm not seeing a benefit here in allowing that. And history says we
change something around mst handling, it'll break something somewhere.
-Daniel

>
> > > > > The check
> > > > > for that also makes driver internal modesets more cumbersome where we
> > > > > need to add exemptions for the cases where we do need to allow the
> > > > > modeset even for unregistered connectors. One such case is the
> > > > > restoration of the mode during resume.
> > > >
> > > > Yeah this one actually makes sense to me. We could still keep this check
> > > > here, but for the atomic ioctl only when called from userspace. But iirc
> > > > Lyude also said she has some plans here, so no idea whether that all fits.
> > > >
> > > > > Simplify things by removing the unneeded check. I can't see how
> > > > > modesetting an unregistered connector can cause any problem and the race
> > > > > (described in the code comment) can anyway result in such a modeset (if
> > > > > the connector is unregistered right after the check).
> > > >
> > > > Not saying we don't need this, but there's fairly enormous amounts of
> > > > history behind all this stuff, and lots of discussions. Would be good to
> > > > at least reference those, so we have a good story for when this then all
> > > > goes wrong again.
> > >
> > > I still don't see why this check is needed. There is no justification
> > > for it - besides the original reason for it as discussed above about the
> > > refcounting problem, which is solved now - so I think we should remove
> > > it, instead of just making it a special case for the user space modeset.
> > >
> > > As I wrote a user space modeset can end up anyway doing a modeset on an
> > > unregistered connector when the unregistering - by MST core - happens just
> > > right after the check.
> >
> > Yup. Always been like that.
> > -Daniel
> >
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > > > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index 2e0cb4246cbd..e94e69483498 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > >                 return 0;
> > > > >         }
> > > > >
> > > > > -       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > -                                                  new_connector_state->crtc);
> > > > > -       /*
> > > > > -        * For compatibility with legacy users, we want to make sure that
> > > > > -        * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > > > -        * which would result in anything else must be considered invalid, to
> > > > > -        * avoid turning on new displays on dead connectors.
> > > > > -        *
> > > > > -        * Since the connector can be unregistered at any point during an
> > > > > -        * atomic check or commit, this is racy. But that's OK: all we care
> > > > > -        * about is ensuring that userspace can't do anything but shut off the
> > > > > -        * display on a connector that was destroyed after it's been notified,
> > > > > -        * not before.
> > > > > -        *
> > > > > -        * Additionally, we also want to ignore connector registration when
> > > > > -        * we're trying to restore an atomic state during system resume since
> > > > > -        * there's a chance the connector may have been destroyed during the
> > > > > -        * process, but it's better to ignore that then cause
> > > > > -        * drm_atomic_helper_resume() to fail.
> > > > > -        */
> > > > > -       if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > > > -           crtc_state->active) {
> > > > > -               DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > > > -                                connector->base.id, connector->name);
> > > > > -               return -EINVAL;
> > > > > -       }
> > > > > -
> > > > >         funcs = connector->helper_private;
> > > > >
> > > > >         if (funcs->atomic_best_encoder)
> > > > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > >
> > > > >         set_best_encoder(state, new_connector_state, new_encoder);
> > > > >
> > > > > +       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > +                                                  new_connector_state->crtc);
> > > > >         crtc_state->connectors_changed = true;
> > > > >
> > > > >         DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

On Mon, May 20, 2019 at 10:15:20PM +0200, Daniel Vetter wrote:
> On Mon, May 20, 2019 at 10:06 PM Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, May 20, 2019 at 09:23:00PM +0200, Daniel Vetter wrote:
> > > On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> > > > On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > > > > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > > > > We allowed modesetting an unregistered connector only in the case the
> > > > > > mode is getting disabled on the connector.
> > > > > >
> > > > > > The reason for this check was the lack of proper refcounting for the
> > > > > > backing memory objects. That problem has been solved meanwhile so there
> > > > > > is no reason any more to reject the modesetting in general.
> > > > >
> > > > > I'm not parsing this at all ... maybe references to the commits that fix
> > > > > this? Or do you mean the refcounting work for all the things hanging of
> > > > > connectors, including the entire mst tree?
> > > >
> > > > Yes the check was added to solve the issue related to the removal of MST
> > > > connectors that could happen asynchronously wrt. a modeset referring to
> > > > that MST connector.  That could happen since the MST core doesn't hold
> > > > any locks (for instance the connection_mutex) during removing an MST
> > > > connector that would prevent doing a modeset at the same time.
> > > >
> > > > Adding the refcounting for such MST connectors (via the
> > > > drm_connector_get()/drm_connector_put()) got rid of the above problem.
> > >
> > > We added the check way after that stuff landed. Before all the connector
> > > reworking connectors were forcefully disabled by the kernel. The idea
> > > behind this check is to make sure that that userspace notices a connector
> > > is gone (only thing that's not allowed is enabling it, you can keep
> > > pageflipping). I think we've always had behaviour like ever since mst (all
> > > userspace has some "oops mst connector probably gone" failure catching
> > > around modesets).
> >
> > Right, pageflipping works.
> >
> > > So no idea what all blows up if we stop catching userspace this way.
> > >
> > > Now very much possible I'm getting all this wrong again or missing
> > > something, this stuff is often way over my head. But I'm really vary of
> > > breaking userspace here. E.g. just the drm_connector_get/put lifetime
> > > changes results in some userspace breaking if you unplug/replug fast
> > > enough, because then it doesn't notice the connector change anymore. I
> > > couldn't figure out a way to paper over that regression without
> > > reintroduce the rather broken and oops-prone old connector lifetime
> > > management.
> >
> > Yes, but what is the the actual description of the failing scenario? I
> > can't see how anything can go wrong without this check. The resume time
> > restoration modeset may have to act in the same way on an old connector.
> 
> That's what the !state->duplicated thing is meant to check for btw.
> 
> > I don't understand how userspace would not notice the connector change.
> > It will get a hotplug uevent in response to which it would have to do a
> > detect which returns to it the updated information about the new MST
> > connector tree.
> 
> uevent handling can take positively forever, at which point the new
> connector could already be plugged in, and then userspace makes a mess
> aliasing the two since the path property matches. Just needs a wobbly
> cable. This is the issue with the "fixed" lifetime management, and I'm
> wary of breaking more stuff.

Yea, processing can be delayed arbitrarily, the corresponding
GETRESOURCES/GETCONNECTOR should still return to the userspace the
correct information to do right thing, even if the path property
matches: the connector ID for the old and new connector will be
different and the GETCONNECTOR for the old ID will return properly that
this old connector is already disconnected (see intel_dp_mst_detect()).
So I don't see how userspace could mess up things. Obviously if user
space is buggy, then well, it is just buggy and then that user space bug
would need to be fixed instead of papering over such problems in the
kernel.

> Other way round: What do we gain if userspace is allowed to turn on a
> connector again which doesn't exist and will not ever show any pixels?
> I'm not seeing a benefit here in allowing that. And history says we
> change something around mst handling, it'll break something somewhere.

Let's then describe the actual reason for this check, that description
is missing.

We should remove this check to simplify things if there isn't an actual
need for it. Userspace can do already a modeset on connectors in general
that are not in a connected state. So this special casing - for MST
connectors only - is a bad idea if there is no reason for special casing
it. If it's there to paper over some user space bug that user space bug
should be fixed instead of adding this special casing which adds
complexity to the driver.

The only justification for a check in the driver if something could blow
up in the kernel without that check. Nothing can blow up in the kernel
without this check. Every other failure scenario should be handled/fixed
in userspace.

> -Daniel
> 
> >
> > > > > > The check
> > > > > > for that also makes driver internal modesets more cumbersome where we
> > > > > > need to add exemptions for the cases where we do need to allow the
> > > > > > modeset even for unregistered connectors. One such case is the
> > > > > > restoration of the mode during resume.
> > > > >
> > > > > Yeah this one actually makes sense to me. We could still keep this check
> > > > > here, but for the atomic ioctl only when called from userspace. But iirc
> > > > > Lyude also said she has some plans here, so no idea whether that all fits.
> > > > >
> > > > > > Simplify things by removing the unneeded check. I can't see how
> > > > > > modesetting an unregistered connector can cause any problem and the race
> > > > > > (described in the code comment) can anyway result in such a modeset (if
> > > > > > the connector is unregistered right after the check).
> > > > >
> > > > > Not saying we don't need this, but there's fairly enormous amounts of
> > > > > history behind all this stuff, and lots of discussions. Would be good to
> > > > > at least reference those, so we have a good story for when this then all
> > > > > goes wrong again.
> > > >
> > > > I still don't see why this check is needed. There is no justification
> > > > for it - besides the original reason for it as discussed above about the
> > > > refcounting problem, which is solved now - so I think we should remove
> > > > it, instead of just making it a special case for the user space modeset.
> > > >
> > > > As I wrote a user space modeset can end up anyway doing a modeset on an
> > > > unregistered connector when the unregistering - by MST core - happens just
> > > > right after the check.
> > >
> > > Yup. Always been like that.
> > > -Daniel
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > > > > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > index 2e0cb4246cbd..e94e69483498 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > >                 return 0;
> > > > > >         }
> > > > > >
> > > > > > -       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > -                                                  new_connector_state->crtc);
> > > > > > -       /*
> > > > > > -        * For compatibility with legacy users, we want to make sure that
> > > > > > -        * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > > > > -        * which would result in anything else must be considered invalid, to
> > > > > > -        * avoid turning on new displays on dead connectors.
> > > > > > -        *
> > > > > > -        * Since the connector can be unregistered at any point during an
> > > > > > -        * atomic check or commit, this is racy. But that's OK: all we care
> > > > > > -        * about is ensuring that userspace can't do anything but shut off the
> > > > > > -        * display on a connector that was destroyed after it's been notified,
> > > > > > -        * not before.
> > > > > > -        *
> > > > > > -        * Additionally, we also want to ignore connector registration when
> > > > > > -        * we're trying to restore an atomic state during system resume since
> > > > > > -        * there's a chance the connector may have been destroyed during the
> > > > > > -        * process, but it's better to ignore that then cause
> > > > > > -        * drm_atomic_helper_resume() to fail.
> > > > > > -        */
> > > > > > -       if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > > > > -           crtc_state->active) {
> > > > > > -               DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > > > > -                                connector->base.id, connector->name);
> > > > > > -               return -EINVAL;
> > > > > > -       }
> > > > > > -
> > > > > >         funcs = connector->helper_private;
> > > > > >
> > > > > >         if (funcs->atomic_best_encoder)
> > > > > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > >
> > > > > >         set_best_encoder(state, new_connector_state, new_encoder);
> > > > > >
> > > > > > +       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > +                                                  new_connector_state->crtc);
> > > > > >         crtc_state->connectors_changed = true;
> > > > > >
> > > > > >         DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Daniel Vetter]

On Mon, May 20, 2019 at 10:51 PM Imre Deak <imre.deak@intel.com> wrote:
>
> On Mon, May 20, 2019 at 10:15:20PM +0200, Daniel Vetter wrote:
> > On Mon, May 20, 2019 at 10:06 PM Imre Deak <imre.deak@intel.com> wrote:
> > > On Mon, May 20, 2019 at 09:23:00PM +0200, Daniel Vetter wrote:
> > > > On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> > > > > On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > > > > > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > > > > > We allowed modesetting an unregistered connector only in the case the
> > > > > > > mode is getting disabled on the connector.
> > > > > > >
> > > > > > > The reason for this check was the lack of proper refcounting for the
> > > > > > > backing memory objects. That problem has been solved meanwhile so there
> > > > > > > is no reason any more to reject the modesetting in general.
> > > > > >
> > > > > > I'm not parsing this at all ... maybe references to the commits that fix
> > > > > > this? Or do you mean the refcounting work for all the things hanging of
> > > > > > connectors, including the entire mst tree?
> > > > >
> > > > > Yes the check was added to solve the issue related to the removal of MST
> > > > > connectors that could happen asynchronously wrt. a modeset referring to
> > > > > that MST connector.  That could happen since the MST core doesn't hold
> > > > > any locks (for instance the connection_mutex) during removing an MST
> > > > > connector that would prevent doing a modeset at the same time.
> > > > >
> > > > > Adding the refcounting for such MST connectors (via the
> > > > > drm_connector_get()/drm_connector_put()) got rid of the above problem.
> > > >
> > > > We added the check way after that stuff landed. Before all the connector
> > > > reworking connectors were forcefully disabled by the kernel. The idea
> > > > behind this check is to make sure that that userspace notices a connector
> > > > is gone (only thing that's not allowed is enabling it, you can keep
> > > > pageflipping). I think we've always had behaviour like ever since mst (all
> > > > userspace has some "oops mst connector probably gone" failure catching
> > > > around modesets).
> > >
> > > Right, pageflipping works.
> > >
> > > > So no idea what all blows up if we stop catching userspace this way.
> > > >
> > > > Now very much possible I'm getting all this wrong again or missing
> > > > something, this stuff is often way over my head. But I'm really vary of
> > > > breaking userspace here. E.g. just the drm_connector_get/put lifetime
> > > > changes results in some userspace breaking if you unplug/replug fast
> > > > enough, because then it doesn't notice the connector change anymore. I
> > > > couldn't figure out a way to paper over that regression without
> > > > reintroduce the rather broken and oops-prone old connector lifetime
> > > > management.
> > >
> > > Yes, but what is the the actual description of the failing scenario? I
> > > can't see how anything can go wrong without this check. The resume time
> > > restoration modeset may have to act in the same way on an old connector.
> >
> > That's what the !state->duplicated thing is meant to check for btw.
> >
> > > I don't understand how userspace would not notice the connector change.
> > > It will get a hotplug uevent in response to which it would have to do a
> > > detect which returns to it the updated information about the new MST
> > > connector tree.
> >
> > uevent handling can take positively forever, at which point the new
> > connector could already be plugged in, and then userspace makes a mess
> > aliasing the two since the path property matches. Just needs a wobbly
> > cable. This is the issue with the "fixed" lifetime management, and I'm
> > wary of breaking more stuff.
>
> Yea, processing can be delayed arbitrarily, the corresponding
> GETRESOURCES/GETCONNECTOR should still return to the userspace the
> correct information to do right thing, even if the path property
> matches: the connector ID for the old and new connector will be
> different and the GETCONNECTOR for the old ID will return properly that
> this old connector is already disconnected (see intel_dp_mst_detect()).
> So I don't see how userspace could mess up things. Obviously if user
> space is buggy, then well, it is just buggy and then that user space bug
> would need to be fixed instead of papering over such problems in the
> kernel.

Uh that's not how this "kernel change caused a regression" thing
works. Dave just dropped a rant about this and i915 recently ...

> > Other way round: What do we gain if userspace is allowed to turn on a
> > connector again which doesn't exist and will not ever show any pixels?
> > I'm not seeing a benefit here in allowing that. And history says we
> > change something around mst handling, it'll break something somewhere.
>
> Let's then describe the actual reason for this check, that description
> is missing.
>
> We should remove this check to simplify things if there isn't an actual
> need for it. Userspace can do already a modeset on connectors in general
> that are not in a connected state. So this special casing - for MST
> connectors only - is a bad idea if there is no reason for special casing
> it. If it's there to paper over some user space bug that user space bug
> should be fixed instead of adding this special casing which adds
> complexity to the driver.
>
> The only justification for a check in the driver if something could blow
> up in the kernel without that check. Nothing can blow up in the kernel
> without this check. Every other failure scenario should be handled/fixed
> in userspace.

So I thought you need this because it's a pain on resume, but that
should be handled with the state->duplicated check. I guess I still
don't understand why you want to nuke this check, it seemed like it's
getting in the way somewhere.

If it's not getting in the way then imo let's not touch stuff, I'm too
much burned child in this area.
-Daniel

> > -Daniel
> >
> > >
> > > > > > > The check
> > > > > > > for that also makes driver internal modesets more cumbersome where we
> > > > > > > need to add exemptions for the cases where we do need to allow the
> > > > > > > modeset even for unregistered connectors. One such case is the
> > > > > > > restoration of the mode during resume.
> > > > > >
> > > > > > Yeah this one actually makes sense to me. We could still keep this check
> > > > > > here, but for the atomic ioctl only when called from userspace. But iirc
> > > > > > Lyude also said she has some plans here, so no idea whether that all fits.
> > > > > >
> > > > > > > Simplify things by removing the unneeded check. I can't see how
> > > > > > > modesetting an unregistered connector can cause any problem and the race
> > > > > > > (described in the code comment) can anyway result in such a modeset (if
> > > > > > > the connector is unregistered right after the check).
> > > > > >
> > > > > > Not saying we don't need this, but there's fairly enormous amounts of
> > > > > > history behind all this stuff, and lots of discussions. Would be good to
> > > > > > at least reference those, so we have a good story for when this then all
> > > > > > goes wrong again.
> > > > >
> > > > > I still don't see why this check is needed. There is no justification
> > > > > for it - besides the original reason for it as discussed above about the
> > > > > refcounting problem, which is solved now - so I think we should remove
> > > > > it, instead of just making it a special case for the user space modeset.
> > > > >
> > > > > As I wrote a user space modeset can end up anyway doing a modeset on an
> > > > > unregistered connector when the unregistering - by MST core - happens just
> > > > > right after the check.
> > > >
> > > > Yup. Always been like that.
> > > > -Daniel
> > > >
> > > > >
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > > > > > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > index 2e0cb4246cbd..e94e69483498 100644
> > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > > >                 return 0;
> > > > > > >         }
> > > > > > >
> > > > > > > -       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > > -                                                  new_connector_state->crtc);
> > > > > > > -       /*
> > > > > > > -        * For compatibility with legacy users, we want to make sure that
> > > > > > > -        * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > > > > > -        * which would result in anything else must be considered invalid, to
> > > > > > > -        * avoid turning on new displays on dead connectors.
> > > > > > > -        *
> > > > > > > -        * Since the connector can be unregistered at any point during an
> > > > > > > -        * atomic check or commit, this is racy. But that's OK: all we care
> > > > > > > -        * about is ensuring that userspace can't do anything but shut off the
> > > > > > > -        * display on a connector that was destroyed after it's been notified,
> > > > > > > -        * not before.
> > > > > > > -        *
> > > > > > > -        * Additionally, we also want to ignore connector registration when
> > > > > > > -        * we're trying to restore an atomic state during system resume since
> > > > > > > -        * there's a chance the connector may have been destroyed during the
> > > > > > > -        * process, but it's better to ignore that then cause
> > > > > > > -        * drm_atomic_helper_resume() to fail.
> > > > > > > -        */
> > > > > > > -       if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > > > > > -           crtc_state->active) {
> > > > > > > -               DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > > > > > -                                connector->base.id, connector->name);
> > > > > > > -               return -EINVAL;
> > > > > > > -       }
> > > > > > > -
> > > > > > >         funcs = connector->helper_private;
> > > > > > >
> > > > > > >         if (funcs->atomic_best_encoder)
> > > > > > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > > >
> > > > > > >         set_best_encoder(state, new_connector_state, new_encoder);
> > > > > > >
> > > > > > > +       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > > +                                                  new_connector_state->crtc);
> > > > > > >         crtc_state->connectors_changed = true;
> > > > > > >
> > > > > > >         DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > > > > > --
> > > > > > > 2.17.1
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

On Mon, May 20, 2019 at 10:59:35PM +0200, Daniel Vetter wrote:
> On Mon, May 20, 2019 at 10:51 PM Imre Deak <imre.deak@intel.com> wrote:
> >
> > On Mon, May 20, 2019 at 10:15:20PM +0200, Daniel Vetter wrote:
> > > On Mon, May 20, 2019 at 10:06 PM Imre Deak <imre.deak@intel.com> wrote:
> > > > On Mon, May 20, 2019 at 09:23:00PM +0200, Daniel Vetter wrote:
> > > > > On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> > > > > > On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > > > > > > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > > > > > > We allowed modesetting an unregistered connector only in the case the
> > > > > > > > mode is getting disabled on the connector.
> > > > > > > >
> > > > > > > > The reason for this check was the lack of proper refcounting for the
> > > > > > > > backing memory objects. That problem has been solved meanwhile so there
> > > > > > > > is no reason any more to reject the modesetting in general.
> > > > > > >
> > > > > > > I'm not parsing this at all ... maybe references to the commits that fix
> > > > > > > this? Or do you mean the refcounting work for all the things hanging of
> > > > > > > connectors, including the entire mst tree?
> > > > > >
> > > > > > Yes the check was added to solve the issue related to the removal of MST
> > > > > > connectors that could happen asynchronously wrt. a modeset referring to
> > > > > > that MST connector.  That could happen since the MST core doesn't hold
> > > > > > any locks (for instance the connection_mutex) during removing an MST
> > > > > > connector that would prevent doing a modeset at the same time.
> > > > > >
> > > > > > Adding the refcounting for such MST connectors (via the
> > > > > > drm_connector_get()/drm_connector_put()) got rid of the above problem.
> > > > >
> > > > > We added the check way after that stuff landed. Before all the connector
> > > > > reworking connectors were forcefully disabled by the kernel. The idea
> > > > > behind this check is to make sure that that userspace notices a connector
> > > > > is gone (only thing that's not allowed is enabling it, you can keep
> > > > > pageflipping). I think we've always had behaviour like ever since mst (all
> > > > > userspace has some "oops mst connector probably gone" failure catching
> > > > > around modesets).
> > > >
> > > > Right, pageflipping works.
> > > >
> > > > > So no idea what all blows up if we stop catching userspace this way.
> > > > >
> > > > > Now very much possible I'm getting all this wrong again or missing
> > > > > something, this stuff is often way over my head. But I'm really vary of
> > > > > breaking userspace here. E.g. just the drm_connector_get/put lifetime
> > > > > changes results in some userspace breaking if you unplug/replug fast
> > > > > enough, because then it doesn't notice the connector change anymore. I
> > > > > couldn't figure out a way to paper over that regression without
> > > > > reintroduce the rather broken and oops-prone old connector lifetime
> > > > > management.
> > > >
> > > > Yes, but what is the the actual description of the failing scenario? I
> > > > can't see how anything can go wrong without this check. The resume time
> > > > restoration modeset may have to act in the same way on an old connector.
> > >
> > > That's what the !state->duplicated thing is meant to check for btw.
> > >
> > > > I don't understand how userspace would not notice the connector change.
> > > > It will get a hotplug uevent in response to which it would have to do a
> > > > detect which returns to it the updated information about the new MST
> > > > connector tree.
> > >
> > > uevent handling can take positively forever, at which point the new
> > > connector could already be plugged in, and then userspace makes a mess
> > > aliasing the two since the path property matches. Just needs a wobbly
> > > cable. This is the issue with the "fixed" lifetime management, and I'm
> > > wary of breaking more stuff.
> >
> > Yea, processing can be delayed arbitrarily, the corresponding
> > GETRESOURCES/GETCONNECTOR should still return to the userspace the
> > correct information to do right thing, even if the path property
> > matches: the connector ID for the old and new connector will be
> > different and the GETCONNECTOR for the old ID will return properly that
> > this old connector is already disconnected (see intel_dp_mst_detect()).
> > So I don't see how userspace could mess up things. Obviously if user
> > space is buggy, then well, it is just buggy and then that user space bug
> > would need to be fixed instead of papering over such problems in the
> > kernel.
> 
> Uh that's not how this "kernel change caused a regression" thing
> works. Dave just dropped a rant about this and i915 recently ...

I still don't see what would depend on the current behavior instead of
acting on the hotplug uevents properly. 

> > > Other way round: What do we gain if userspace is allowed to turn on a
> > > connector again which doesn't exist and will not ever show any pixels?
> > > I'm not seeing a benefit here in allowing that. And history says we
> > > change something around mst handling, it'll break something somewhere.
> >
> > Let's then describe the actual reason for this check, that description
> > is missing.
> >
> > We should remove this check to simplify things if there isn't an actual
> > need for it. Userspace can do already a modeset on connectors in general
> > that are not in a connected state. So this special casing - for MST
> > connectors only - is a bad idea if there is no reason for special casing
> > it. If it's there to paper over some user space bug that user space bug
> > should be fixed instead of adding this special casing which adds
> > complexity to the driver.
> >
> > The only justification for a check in the driver if something could blow
> > up in the kernel without that check. Nothing can blow up in the kernel
> > without this check. Every other failure scenario should be handled/fixed
> > in userspace.
> 
> So I thought you need this because it's a pain on resume, but that
> should be handled with the state->duplicated check. I guess I still
> don't understand why you want to nuke this check, it seemed like it's
> getting in the way somewhere.
> 
> If it's not getting in the way then imo let's not touch stuff, I'm too
> much burned child in this area.

It's a pain to maintain such papering overs in the kernel, that nobody
knows what purpose they serve. What I'm asking here is to preserve this
check here only if there is a purpose for it along with a precise
description of its purpose. If noone can give an explanation for it -
or how it can lead to a regression - then yes, let's remove it to
simplify things.

> -Daniel
> 
> > > -Daniel
> > >
> > > >
> > > > > > > > The check
> > > > > > > > for that also makes driver internal modesets more cumbersome where we
> > > > > > > > need to add exemptions for the cases where we do need to allow the
> > > > > > > > modeset even for unregistered connectors. One such case is the
> > > > > > > > restoration of the mode during resume.
> > > > > > >
> > > > > > > Yeah this one actually makes sense to me. We could still keep this check
> > > > > > > here, but for the atomic ioctl only when called from userspace. But iirc
> > > > > > > Lyude also said she has some plans here, so no idea whether that all fits.
> > > > > > >
> > > > > > > > Simplify things by removing the unneeded check. I can't see how
> > > > > > > > modesetting an unregistered connector can cause any problem and the race
> > > > > > > > (described in the code comment) can anyway result in such a modeset (if
> > > > > > > > the connector is unregistered right after the check).
> > > > > > >
> > > > > > > Not saying we don't need this, but there's fairly enormous amounts of
> > > > > > > history behind all this stuff, and lots of discussions. Would be good to
> > > > > > > at least reference those, so we have a good story for when this then all
> > > > > > > goes wrong again.
> > > > > >
> > > > > > I still don't see why this check is needed. There is no justification
> > > > > > for it - besides the original reason for it as discussed above about the
> > > > > > refcounting problem, which is solved now - so I think we should remove
> > > > > > it, instead of just making it a special case for the user space modeset.
> > > > > >
> > > > > > As I wrote a user space modeset can end up anyway doing a modeset on an
> > > > > > unregistered connector when the unregistering - by MST core - happens just
> > > > > > right after the check.
> > > > >
> > > > > Yup. Always been like that.
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > > -Daniel
> > > > > > >
> > > > > > > >
> > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > > > > > > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > index 2e0cb4246cbd..e94e69483498 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > > > >                 return 0;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > -       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > > > -                                                  new_connector_state->crtc);
> > > > > > > > -       /*
> > > > > > > > -        * For compatibility with legacy users, we want to make sure that
> > > > > > > > -        * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > > > > > > -        * which would result in anything else must be considered invalid, to
> > > > > > > > -        * avoid turning on new displays on dead connectors.
> > > > > > > > -        *
> > > > > > > > -        * Since the connector can be unregistered at any point during an
> > > > > > > > -        * atomic check or commit, this is racy. But that's OK: all we care
> > > > > > > > -        * about is ensuring that userspace can't do anything but shut off the
> > > > > > > > -        * display on a connector that was destroyed after it's been notified,
> > > > > > > > -        * not before.
> > > > > > > > -        *
> > > > > > > > -        * Additionally, we also want to ignore connector registration when
> > > > > > > > -        * we're trying to restore an atomic state during system resume since
> > > > > > > > -        * there's a chance the connector may have been destroyed during the
> > > > > > > > -        * process, but it's better to ignore that then cause
> > > > > > > > -        * drm_atomic_helper_resume() to fail.
> > > > > > > > -        */
> > > > > > > > -       if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > > > > > > -           crtc_state->active) {
> > > > > > > > -               DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > > > > > > -                                connector->base.id, connector->name);
> > > > > > > > -               return -EINVAL;
> > > > > > > > -       }
> > > > > > > > -
> > > > > > > >         funcs = connector->helper_private;
> > > > > > > >
> > > > > > > >         if (funcs->atomic_best_encoder)
> > > > > > > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > > > > > > >
> > > > > > > >         set_best_encoder(state, new_connector_state, new_encoder);
> > > > > > > >
> > > > > > > > +       crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > > > +                                                  new_connector_state->crtc);
> > > > > > > >         crtc_state->connectors_changed = true;
> > > > > > > >
> > > > > > > >         DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > > > > > > --
> > > > > > > > 2.17.1
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Imre Deak]

On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> We allowed modesetting an unregistered connector only in the case the
> mode is getting disabled on the connector.
> 
> The reason for this check was the lack of proper refcounting for the
> backing memory objects. That problem has been solved meanwhile so there
> is no reason any more to reject the modesetting in general. The check
> for that also makes driver internal modesets more cumbersome where we
> need to add exemptions for the cases where we do need to allow the
> modeset even for unregistered connectors. One such case is the
> restoration of the mode during resume.
> 
> Simplify things by removing the unneeded check. I can't see how
> modesetting an unregistered connector can cause any problem and the race
> (described in the code comment) can anyway result in such a modeset (if
> the connector is unregistered right after the check).
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Lyude, could you test this change against the DDX fail scenario that the
check was added for originally (based on our IRC discussion)?

Thanks,
Imre

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2e0cb4246cbd..e94e69483498 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(state,
> -						   new_connector_state->crtc);
> -	/*
> -	 * For compatibility with legacy users, we want to make sure that
> -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> -	 * which would result in anything else must be considered invalid, to
> -	 * avoid turning on new displays on dead connectors.
> -	 *
> -	 * Since the connector can be unregistered at any point during an
> -	 * atomic check or commit, this is racy. But that's OK: all we care
> -	 * about is ensuring that userspace can't do anything but shut off the
> -	 * display on a connector that was destroyed after it's been notified,
> -	 * not before.
> -	 *
> -	 * Additionally, we also want to ignore connector registration when
> -	 * we're trying to restore an atomic state during system resume since
> -	 * there's a chance the connector may have been destroyed during the
> -	 * process, but it's better to ignore that then cause
> -	 * drm_atomic_helper_resume() to fail.
> -	 */
> -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> -	    crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> -				 connector->base.id, connector->name);
> -		return -EINVAL;
> -	}
> -
>  	funcs = connector->helper_private;
>  
>  	if (funcs->atomic_best_encoder)
> @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	set_best_encoder(state, new_connector_state, new_encoder);
>  
> +	crtc_state = drm_atomic_get_new_crtc_state(state,
> +						   new_connector_state->crtc);
>  	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for drm: Allow modeset on unregisted connectors unconditionally (rev2)

[Patchwork]

== Series Details ==

Series: drm: Allow modeset on unregisted connectors unconditionally (rev2)
URL   : https://patchwork.freedesktop.org/series/60871/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6110 -> Patchwork_13054
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/

Known issues
------------

  Here are the changes found in Patchwork_13054 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@read_all_entries:
    - fi-ilk-650:         [PASS][1] -> [DMESG-WARN][2] ([fdo#106387])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/fi-ilk-650/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/fi-ilk-650/igt@debugfs_test@read_all_entries.html

  
#### Possible fixes ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][3] ([fdo#109485]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Warnings ####

  * igt@i915_selftest@live_hangcheck:
    - fi-apl-guc:         [DMESG-FAIL][5] ([fdo#110620]) -> [FAIL][6] ([fdo#110623])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#110620]: https://bugs.freedesktop.org/show_bug.cgi?id=110620
  [fdo#110623]: https://bugs.freedesktop.org/show_bug.cgi?id=110623


Participating hosts (49 -> 43)
------------------------------

  Additional (2): fi-icl-u2 fi-gdg-551 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-pnv-d510 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6110 -> Patchwork_13054

  CI_DRM_6110: ae35264eb9f700d5f9856552bf98e20ec2b74739 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4999: eee6e9ea7a7e22a64afc029a696b80da8053bc3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13054: 949e61ed7490fb4da9dd29729084af60a1174d4d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

949e61ed7490 drm: Allow modeset on unregisted connectors unconditionally

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm: Allow modeset on unregisted connectors unconditionally (rev2)

[Patchwork]

== Series Details ==

Series: drm: Allow modeset on unregisted connectors unconditionally (rev2)
URL   : https://patchwork.freedesktop.org/series/60871/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6110_full -> Patchwork_13054_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13054_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_create@create-clear:
    - shard-hsw:          [PASS][1] -> [INCOMPLETE][2] ([fdo#103540] / [fdo#110401])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-hsw6/igt@gem_create@create-clear.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-hsw1/igt@gem_create@create-clear.html

  * igt@gem_eio@in-flight-suspend:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([fdo#110667])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-glk5/igt@gem_eio@in-flight-suspend.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-glk5/igt@gem_eio@in-flight-suspend.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#108566])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-apl8/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-apl4/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_mmap_gtt@big-copy:
    - shard-snb:          [PASS][7] -> [INCOMPLETE][8] ([fdo#105411]) +4 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-snb2/igt@gem_mmap_gtt@big-copy.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-snb7/igt@gem_mmap_gtt@big-copy.html

  * igt@gem_pwrite@big-gtt-fbr:
    - shard-kbl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#103665]) +5 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-kbl7/igt@gem_pwrite@big-gtt-fbr.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-kbl2/igt@gem_pwrite@big-gtt-fbr.html

  * igt@gem_pwrite@small-cpu-backwards:
    - shard-glk:          [PASS][11] -> [INCOMPLETE][12] ([fdo#103359] / [k.org#198133]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-glk8/igt@gem_pwrite@small-cpu-backwards.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-glk4/igt@gem_pwrite@small-cpu-backwards.html

  * igt@gem_pwrite@small-cpu-random:
    - shard-hsw:          [PASS][13] -> [INCOMPLETE][14] ([fdo#103540]) +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-hsw7/igt@gem_pwrite@small-cpu-random.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-hsw5/igt@gem_pwrite@small-cpu-random.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [PASS][15] -> [DMESG-WARN][16] ([fdo#108686])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-glk8/igt@gem_tiled_swapping@non-threaded.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-glk1/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_pm_rpm@reg-read-ioctl:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#107807])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl9/igt@i915_pm_rpm@reg-read-ioctl.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl2/igt@i915_pm_rpm@reg-read-ioctl.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-skl:          [PASS][19] -> [INCOMPLETE][20] ([fdo#104108])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145] / [fdo#110403])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@basic-small-bo:
    - shard-hsw:          [INCOMPLETE][23] ([fdo#103540]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-hsw8/igt@gem_mmap_gtt@basic-small-bo.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-hsw6/igt@gem_mmap_gtt@basic-small-bo.html

  * igt@gem_mmap_gtt@forked-basic-small-copy-xy:
    - shard-kbl:          [INCOMPLETE][25] ([fdo#103665]) -> [PASS][26] +3 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-kbl4/igt@gem_mmap_gtt@forked-basic-small-copy-xy.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-kbl7/igt@gem_mmap_gtt@forked-basic-small-copy-xy.html

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-glk:          [INCOMPLETE][27] ([fdo#103359] / [k.org#198133]) -> [PASS][28] +3 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-glk4/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-glk6/igt@gem_ppgtt@blt-vs-render-ctx0.html
    - shard-snb:          [INCOMPLETE][29] ([fdo#105411]) -> [PASS][30] +4 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-snb7/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-snb4/igt@gem_ppgtt@blt-vs-render-ctx0.html

  * igt@i915_suspend@sysfs-reader:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-apl4/igt@i915_suspend@sysfs-reader.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-apl4/igt@i915_suspend@sysfs-reader.html

  * igt@kms_busy@basic-flip-a:
    - shard-snb:          [SKIP][33] ([fdo#109271] / [fdo#109278]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-snb2/igt@kms_busy@basic-flip-a.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-snb7/igt@kms_busy@basic-flip-a.html

  * {igt@kms_cursor_crc@pipe-a-cursor-suspend}:
    - shard-skl:          [INCOMPLETE][35] -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled:
    - shard-skl:          [FAIL][37] ([fdo#103184] / [fdo#103232]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl4/igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl1/igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled:
    - shard-snb:          [SKIP][39] ([fdo#109271]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-snb2/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-snb7/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-c:
    - shard-skl:          [FAIL][41] ([fdo#103191]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl2/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl3/igt@kms_pipe_crc_basic@read-crc-pipe-c.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][43] ([fdo#108145] / [fdo#110403]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-skl:          [INCOMPLETE][45] ([fdo#104108]) -> [PASS][46] +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-skl2/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-skl6/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html

  
#### Warnings ####

  * igt@gem_pwrite@huge-cpu-forwards:
    - shard-kbl:          [INCOMPLETE][47] ([fdo#103665]) -> [SKIP][48] ([fdo#109271]) +1 similar issue
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-kbl3/igt@gem_pwrite@huge-cpu-forwards.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-kbl1/igt@gem_pwrite@huge-cpu-forwards.html

  * igt@gem_userptr_blits@coherency-sync:
    - shard-kbl:          [SKIP][49] ([fdo#109271]) -> [INCOMPLETE][50] ([fdo#103665])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-kbl1/igt@gem_userptr_blits@coherency-sync.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-kbl3/igt@gem_userptr_blits@coherency-sync.html
    - shard-glk:          [SKIP][51] ([fdo#109271]) -> [INCOMPLETE][52] ([fdo#103359] / [k.org#198133])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6110/shard-glk3/igt@gem_userptr_blits@coherency-sync.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/shard-glk4/igt@gem_userptr_blits@coherency-sync.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110401]: https://bugs.freedesktop.org/show_bug.cgi?id=110401
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110667]: https://bugs.freedesktop.org/show_bug.cgi?id=110667
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-iclb 


Build changes
-------------

  * Linux: CI_DRM_6110 -> Patchwork_13054

  CI_DRM_6110: ae35264eb9f700d5f9856552bf98e20ec2b74739 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4999: eee6e9ea7a7e22a64afc029a696b80da8053bc3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13054: 949e61ed7490fb4da9dd29729084af60a1174d4d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13054/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx