Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 15, 2014 at 12:35:17PM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> > On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> > > On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> > > ...
> > > > 
> > > > > +	};
> > > > > +	bool visible;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > > > +					      &src, &dest, &clip,
> > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > +					      false, true, &visible);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (!visible)
> > > > > +		return intel_primary_plane_disable(plane);
> > > > 
> > > > Here we unpin the old fb...
> > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > > > +	 * updating the fb pointer, and returning without touching the
> > > > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > > > +	 * turn on the display with all planes setup as desired.
> > > > > +	 */
> > > > > +	if (!crtc->enabled)
> > > > > +		/* Pin and return without programming hardware */
> > > > > +		return intel_pin_and_fence_fb_obj(dev,
> > > > > +						  to_intel_framebuffer(fb)->obj,
> > > > > +						  NULL);
> > > > 
> > > > ...but here we just pin the new fb and leave the old also pinned?
> > > > 
> > > > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > > > plane doesn't seem to make sense. We also need to remember that set_base
> > > > will always unpin the old_fb. So if something can result in set_base
> > > > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > > > doodoo.
> > > 
> > > Right, I guess we need to unpin the old fb, if any, here as well in case
> > > they perform several setplane() calls while the crtc is disabled.
> > > 
> > > Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> > > do setcrtc(fb = -1), then it should keep whatever fb we've already
> > > pinned via the setplane.  If they provide a new fb, then the pinning
> > > we're doing here will get unpinned by the set_base that gets called.
> > > 
> > > I don't see a way that you can hit set_base with an unpinned
> > > old_fb!=NULL (since the disable plane case farther up also clears
> > > primary->fb).
> > 
> > But it doesn't.
> > 
> 
> Ah, you're right.  I was conflating explicit disables (fb=0) with
> implicit disables (clipped to invisible).  I think the v7 I just sent
> should handle this properly...for the implicit disable case we leave the
> fb pinned and pointed to by primary->fb.  So when we switch to another
> fb (or explicitly disable with fb=0), we should unpin it properly.

Do we have proper coverage for this fun in our primary plane helper tests?
This is the kind of complexity that freaks me out ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch