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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC