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

Daniel Vetter
2014-05-15T20:49:57Z

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