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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
