Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.
>>
>> 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@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@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. :)
>>  		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. :)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx