On Wed, Aug 1, 2012 at 6:36 AM, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Tue, Jul 31, 2012 at 02:28:29PM -0500, Rob Clark wrote: >> 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() > > Your patch would still significantly change the behavior of > drm_mode_rmfb(). Currently it disables all planes and crtcs which > currently use the fb, and it removes the fb id from the idr so that > no new users of the fb can appear afterwards. > > Not that I really like the current behaviour of drm_mode_rmfb(), but > it's been like that always, so changing it doesn't seem acceptable. yeah, I'm working on an update that decouples the crtc/plane shutdown from deleting the fb, which should address these issues BR, -R > -- > Ville Syrjälä > Intel OTC > -- > 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
