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

[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?

-- 
Ville Syrjälä
Intel OTC