Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> 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.
Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check
won't
be invoked. Hence it's the most accurate way of making sure that
crtc enabled <=> primary plane bound.
If the check was done in the primary plane, an atomic modeset could enable
the crtc without enabling the primary plane, which shouldn't be allowed but
a plane check won't catch it.
This has been a bug in simple-kms-helper, fixed in the below commit:
commit 765831dc27ab141b3a0be1ab55b922b012427902
Author: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
Date: Wed Jul 12 10:13:29 2017 +0200
drm/simple-kms-helper: Fix the check for the mismatch between plane and
CRTC enabled.
> 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.lankho...@linux.intel.com>
>>>> Cc: Sean Paul <seanp...@chromium.org>
>>>> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
>>>> ---
>>>> 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.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
