Re: [PATCH 9/9] drm/i915: Don't re-enable an explicitly disabled primary plane due to sprite coverage changes

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Mar 11, 2015 at 12:09:24PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 11, 2015 at 11:00:32AM +0100, Daniel Vetter wrote:
> > On Tue, Mar 10, 2015 at 01:15:29PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > When the sprite is covering the entire pipe (and color keying is not
> > > enabled) we currently try to automagically disable the primary plane
> > > which is fully covered by the sprite.
> > > 
> > > Now that .crtc_disable() will simply disable planes by setting the
> > > state->visible to false, the sprite code will actually end up
> > > re-enabling the primary after it got disabled, and then we proceed to
> > > turn off the pipe and are greeted with some warnings from
> > > assert_plane_disabled().
> > > 
> > > The code doing the automagic disable of covered planes needs to
> > > rewritten to do things correctly as part of the atomic update (or simply
> > > removed), but in the meantime add a a bit of duct tape and simply have
> > > the sprite code check the primary plane's state->visible before
> > > re-enabling it.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index a828736..7286497 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1287,7 +1287,9 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > >  	intel_plane->obj = obj;
> > >  
> > >  	if (intel_crtc->active) {
> > > -		intel_crtc->primary_enabled = !state->hides_primary;
> > > +		intel_crtc->primary_enabled =
> > > +			to_intel_plane_state(crtc->primary->state)->visible &&
> > > +			!state->hides_primary;
> > 
> > Can't we just nuke intel_crtc->primary_enabled with all your work to tread
> > state through functions correctly?
> 
> Not if we want to keep this magic "disable primary when sprite covers
> it" code.
> 
> I think ideally we'd do the .atomic_check() for all planes, and then
> once all planes are clipped and whatnot, we could check which planes
> are fully covered by something opaque and make them invisible. Though
> that would perhaps mean that we always have to .atomic_check() all the
> planes even if only one of them changed.

Yeah that's what I've meant with removing primary_enabled - this should
flow into the compuation of ->visible for the primary plane. Keeping
random state out of state objects will be serious trouble.

Also if we do this the wm code will grow clue about the situation and stop
allocating fifo space for the primary plane in that case too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx