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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> -Daniel
> 
> >  
> >  		if (state->visible) {
> >  			crtc_x = state->dst.x1;
> > -- 
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx