Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, Jul 31, 2012 at 02:28:29PM -0500, Rob Clark wrote: On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark rob.cl...@linaro.org wrote: On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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. -- Ville Syrjälä Intel OTC -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Wed, Aug 1, 2012 at 6:36 AM, Ville Syrjälä ville.syrj...@linux.intel.com 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 ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark rob.cl...@linaro.org wrote: On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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? -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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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?) BR, -R -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark rob.cl...@linaro.org wrote: On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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. -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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark rob.cl...@linaro.org wrote: On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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. fwiw, my line of reasoning was: from userspace perspective, once you do drm_mode_rmfb(), the fb is gone, so I didn't change where it gets removed from the list of fb's or anything like that. Instead I just left it so that a driver could, if it wants, take an extra ref temporarily to the fb to keep it around for a bit. I didn't change any of the existing drivers, other than update the to call drm_framebuffer_unreference() instead of fb-funcs-delete() directly, because I didn't want to break anything in existing drivers, and I figured the new behavior was better as an opt-in by individual drivers when they want to take advantage of refcnt'ing. But if you have suggestions about related cleanups that should be made, I'm willing to take that on. Anyways, like I mentioned, I'm still a little ways from being ready to submit anything other than RFC patches in omapdrm to use fb refcnt'ing, so this isn't something that needs to be merged immediately. But it seemed like low risk and like it would be something that other drivers could take advantage of, so I figured it was worth sending this patch now. 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm: refcnt drm_framebuffer
On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark rob.cl...@linaro.org wrote: On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark rob.cl...@linaro.org wrote: From: Rob Clark r...@ti.com 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
