| Message ID | 20190415080010.23466-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm: Do not allow to set NOFB for active primary plane | expand |
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@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. ~Maarten
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@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. > > ~Maarten > >
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@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
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,
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@intel.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++ 1 file changed, 9 insertions(+)