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

[Matt Roper <matthew.d.roper@intel.com>]

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.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795