On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: > On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@xxxxxxxxxx> wrote: >> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: >> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@xxxxxxxxxx> wrote: >> >> From: Rob Clark <rob@xxxxxx> >> >> >> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold >> >> a ref to the fb when disabling a pipe until the next vblank, this >> >> avoids the need to make disabling an overlay synchronous. This is a >> >> problem that shows up when userspace is using a drm plane to >> >> implement a hw cursor.. making overlay disable synchronous causes >> >> a performance problem when x11 is rapidly enabling/disabling the >> >> hw cursor. But not making it synchronous opens up a race condition >> >> for crashing if userspace turns around and immediately deletes the >> >> fb. Refcnt'ing the fb makes it possible to solve this problem. >> > >> > Presumably you have a follow-on patch putting the new refcnt to use so >> > that we can judge whether you truly need refcnting on the fb itself in >> > addition to the refcnted object and the various hw bookkeeping that >> > needs to be performed? >> >> Yes, I do.. although it is a bit experimental at this point, so not >> really ready to be submitted as anything other than an RFC.. it is >> part of omapdrm kms re-write to use dispc directly rather than go thru >> omapdss. (With omapdss we don't hit this issue because disabling >> overlays is forced to be synchronous.. so instead we have the >> performance problem I mentioned.) >> >> I *could* just rely on the GEM refcnt, but that gets messier when you >> take into account multi-planar formats. I suppose I also could have >> my own internal refcnt'd object to hold the set of GEM objects >> associated w/ the fb, but, well, that seems a bit silly. And >> refcnt'ing the fb had been mentioned previously as a good thing to do >> (I think it was danvet?) > > Sure, there are a few places in the code that have caused ordering > issues in the past due to lack of refcnting the fb... But since you > haven't fixed up those cases, I'm looking for justification for adding > that extra bit of complexity. Adding a new interface and no users is > just asking for trouble. hmm, I did realize that drm_plane cleanup only happens as a result of drm_framebuffer_cleanup().. which doesn't work too well if the driver is holding a ref to the fb :-/ so I guess at a minimum I need to fix plane cleanup to be part of drm_fb_helper_restore_fbdev_mode() BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
