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

[Daniel Vetter]

On Thu, May 15, 2014 at 01:59:39PM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 10:49:52PM +0200, Daniel Vetter wrote:
> > 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
> 
> Was 'helper' in your question above a typo?  The i-g-t tests I've
> written have been intended for use with this patch (i.e., i915-specific
> primary plane support), so I don't really have any tests that only test
> the lesser, helper-provided functionality (and drivers using the helpers
> shouldn't run into the things Ville is pointing out here because they
> can't disable the primary plane independent of the crtc).
> 
> But assuming you meant the general i-g-t tests, yeah, I also posted a
> slightly updated version so that it now tries to set multiple fb's while
> the crtc is off.  Since crtc=off causes the primary plane to be fully
> clipped and implicitly disabled, it should exercise these cases and
> catch the pin/unpin mistakes that Ville's review caught.

Yeah, s/helper// ;-) For the tests, do you also have some that use the
implicit fb (i.e. fb = -1) with setcrtc? Just kinda for completeness.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch