[PATCH v1] drm: Do not allow to set NOFB for active primary plane

[Daniel Vetter]

On Mon, Apr 15, 2019 at 11:27:25AM +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2019-04-15 at 12:40 +0200, Maarten Lankhorst wrote:
> > Op 15-04-2019 om 10:00 schreef Stanislav Lisovskiy:
> > > There was an issue reported from end users, confirmed
> > > also locally that when user executes xrandr --output <some DP>
> > > --rotate left/right, the other eDP screen gets blank after
> > > rotation.
> > > 
> > > Investigation showed that reason was that primary plane
> > > was for some reason disabled, while cursor plane was still enabled.
> > > After some effort it turns out that userspace might wrongly
> > > assign NOFB to active primary plane for some reason.
> > > 
> > > Then this gets detected in drm_atomic_helper_check_plane_state,
> > > called from ->plane_check and plane gets deactivated, leaving
> > > the screen blank and cursor visible. This can be cured by reboot
> > > or xrandr off/on sequence for that crtc.
> > > 
> > > This patch is proposal to fix the issue by forbiding fb removal
> > > from active primary plane. If one needs to remove fb plane must be
> > > disabled first.
> > > 
> > > Not sure if this is correct, however it fixes the issue at least.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110375
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index ea797d4c82ee..e2f078b302f3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -229,6 +229,15 @@ drm_atomic_set_fb_for_plane(struct
> > > drm_plane_state *plane_state,
> > >  {
> > >  	struct drm_plane *plane = plane_state->plane;
> > >  
> > > +	if (!fb) {
> > > +		if (plane->state->visible && plane->type ==
> > > DRM_PLANE_TYPE_PRIMARY) {
> > > +			DRM_DEBUG_ATOMIC("Not allowed to set [NOFB] for
> > > active"
> > > +					" primary [PLANE:%d:%s] -
> > > disable first",
> > > +					plane->base.id, plane->name);
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > >  	if (fb)
> > >  		DRM_DEBUG_ATOMIC("Set [FB:%d] for [PLANE:%d:%s] state
> > > %p\n",
> > >  				 fb->base.id, plane->base.id, plane-
> > > >name,
> > 
> > If userspace asks to disable the primary plane, but not the crtc then
> > that is not an atomic bug. The bug is in the userspace requesting
> > this (Xorg).
> > 
> > If you want to include such a check, the correct place is at the end
> > of drm_atomic_check_only(), after ->atomic_check(), return -EINVAL
> > there.
> > 
> > But such a patch will not be accepted. It's perfectly acceptable to
> > disable the primary plane for whatever reason, and some of our
> > internal tests do so.
> 
> Well, yeah that's why I was not quite sure, if this is correct.
> However to me the problem seems to be that userspace is disabling
> primary plane without realizing it. For example on my skl I see
> that drm_atomic_helper_check_plane_state complains that primary
> plane has no FB and then disables it.

Yeah we need to figure out where this is coming from. Is this with sna or
-modesetting or something else?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch