[PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Wed, Aug 1, 2012 at 6:36 AM, Ville Syrjälä
<ville.syrjala at 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 <chris at chris-wilson.co.uk> wrote:
>> > On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark at linaro.org> wrote:
>> >> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> >> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark at linaro.org> wrote:
>> >> >> From: Rob Clark <rob at 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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html