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

[Maarten Lankhorst]

Op 06-11-17 om 15:06 schreef Ville Syrjälä:
> On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote:
>> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote:
>>> 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.lankhorst at 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.
>> Hmm. OK that part looks OK. What does seem a bit inconsistent is the
>> fact that we pass can_update_disabled=true, but later on we reject the
>> update when visible==false. The old code would have accepted that
>> because it didn't even call drm_plane_helper_check_state() when the
>> crtc (and thus also the plane) was disabled.
> Actually how does this work at all? If you turn off both the crtc and
> plane, then the plane check will always return -EINVAL on account of
> visible==false?
>
If the crtc is turned off, enabled = false and the primary plane is not in crtc_state->plane_mask.                                                                                                                                                                                                                                                                                                                                                                                                               

Visibility is ignored in this check, that part is handled in plane check that the framebuffer has to span the entire crtc if enabled.