On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> > On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> >> This introduces a slight behavioral change to rmfb. Instead of
> >> disabling a crtc when the primary plane is disabled, we try to
> >> preserve it.
> >>
> >> Apart from old versions of the vmwgfx xorg driver, there is
> >> nothing depending on rmfb disabling a crtc.
> >>
> >> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> >> enabled without plane, so we can do this safely.
The code for those seems a bit inconsistent. The crtc check requires
that the crtc state and plane state match. But the plane check allows
the plane to be enabled w/o the crtc being enabled. I guess it doesn't
matter really since you can't enable the plane without a crtc, and the
crtc check would then catch the case where the crtc would be disabled.
Oh and looks like drm_plane_helper_check_state() is a bit buggy. It
still uses crtc->enabled instead of crtc_state->enable to check the
state of the crtc. I guess to keep drm_plane_helper_check_update()
working we may have to pass in the crtc state manually.
The vmwgfx plane check looks a bit bogus in other ways too. I guess
I'll have to fire off a couple of patches.
> >>
> >> If the atomic commit is rejected by the driver then we will still
> >> fall back to the old behavior and turn off the crtc.
> >>
> >> Changes since v1:
> >> - Restart completely when rmfb with crtc on fails (Sean Paul).
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> ---
> >> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
> >> 1 file changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >> index 2affe53f3fda..f0679468f421 100644
> >> --- a/drivers/gpu/drm/drm_framebuffer.c
> >> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> struct drm_plane *plane;
> >> struct drm_connector *conn;
> >> struct drm_connector_state *conn_state;
> >> - int i, ret = 0;
> >> + int i, ret;
> >> unsigned plane_mask;
> >> + bool disable_crtcs = false;
> >>
> >> - state = drm_atomic_state_alloc(dev);
> >> - if (!state)
> >> - return -ENOMEM;
> >> -
> >> +retry_disable:
> >> drm_modeset_acquire_init(&ctx, 0);
> >> +
> >> + state = drm_atomic_state_alloc(dev);
> >> + if (!state) {
> >> + ret = -ENOMEM;
> >> + goto out;
> >> + }
> >> state->acquire_ctx = &ctx;
> >>
> >> retry:
> >> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> goto unlock;
> >> }
> >>
> >> - if (plane_state->crtc->primary == plane) {
> >> + if (disable_crtcs && plane_state->crtc->primary == plane) {
> >> struct drm_crtc_state *crtc_state;
> >>
> >> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
> >> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> plane->old_fb = plane->fb;
> >> }
> >>
> >> + /* This list is only filled when disable_crtcs is set. */
> >> for_each_new_connector_in_state(state, conn, conn_state, i) {
> > WARN_ON(!disable_crtcs) maybe?
> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :)
It would serve as a way to document that fact, even without the comment.
But I won't insist on it.
> >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> >>
> >> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >>
> >> drm_atomic_state_put(state);
> >>
> >> +out:
> >> drm_modeset_drop_locks(&ctx);
> >> drm_modeset_acquire_fini(&ctx);
> >>
> >> + if (ret == -EINVAL && !disable_crtcs) {
> > Hmm. -EINVAL seems rather specific. Not sure if we could just check for
> > any error?
> >
> > Or... I'm not sure if we have any central place where we do the
> > "can I disable the primary plane w/o disabling the crtc?" check. If we
> > do then we could also add a comment there informing people that the
> > -EINVAL is important.
> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
Oh well. I guess people just have to be careful with their error
values. I suppoe anyone depending on the retry will notice this
issue rather quickly.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
