Op 23-02-17 om 16:24 schreef Sean Paul:
> On Tue, Feb 21, 2017 at 02:51:42PM +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. Since vmwgfx is
>> a legacy driver we can safely only disable the plane with atomic.
>> In the conversion to atomic the driver will reject the config
>> without primary plane.
>>
>> If this commit is rejected by the driver then we will still fall
>> back to the old behavior and turn off the crtc.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 285e1c23e8c9..0d5b0065208e 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>> struct drm_connector *conn;
>> struct drm_connector_state *conn_state;
>> int i, ret = 0;
>> - unsigned plane_mask;
>> + unsigned plane_mask, disable_crtcs = false;
>>
>> state = drm_atomic_state_alloc(dev);
>> if (!state)
>> @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>> goto unlock;
>> }
>>
>> - if (plane_state->crtc->primary == plane) {
>> + /*
>> + * Some drivers do not support keeping crtc active with the
>> + * primary plane disabled. If we fail to commit with -EINVAL
>> + * then we will try to perform the same commit but with all
>> + * crtc's disabled for primary planes as well.
>> + */
>> + 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);
>> @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>> plane->old_fb = plane->fb;
>> }
>>
>> + /* This list is only not empty when disable_crtcs is set. */
>> for_each_connector_in_state(state, conn, conn_state, i) {
>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>>
>> @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>>
>> drm_atomic_state_put(state);
>>
>> + if (ret == -EINVAL && !disable_crtcs) {
>> + disable_crtcs = true;
>> +
>> + state = drm_atomic_state_alloc(dev);
>> + if (state) {
>> + state->acquire_ctx = &ctx;
>> + goto retry;
> Re-using the retry label is a little fishy here. You need to re-allocate state,
> but we don't drop the locks, which makes things a little unbalanced (I realize
> modeset_lock will s/EALREADY/0/).
drm_modeset_lock can be called any arbitrary numer of times. I'm not worried about it. :)
> Perhaps something like this above would be more clear:
>
> + drm_modeset_acquire_init(&ctx, 0);
> +retry:
> + plane_mask = 0;
> + ret = drm_modeset_lock_all_ctx(dev, &ctx);
> + if (ret)
> + goto unlock;
> +
> +retry_disable:
> + state = drm_atomic_state_alloc(dev);
> + if (!state)
> + return -ENOMEM;
> + state->acquire_ctx = &ctx;
> +
>
> Then you can do:
>
>
> + if (ret == -EINVAL && !disable_crtcs) {
> + disable_crtcs = true;
> + goto retry_disable;
> + }
>
> (you would also need to ensure you moved the state_put above the EDEADLK
> handling).
>
> Thoughts?
In general we try to call drm_atomic_state_clear instead of drm_atomic_state_put, this would be a source of confusion for being different from other users of atomic.
~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
