On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx> wrote: > On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: >>> A framebuffer is pinned when it's set as pipe base, so we also need to >>> unpin it when it's destroyed. Some framebuffers are never used as >>> scanout (no mode set on them) so if the pin count is less than one, no >>> unpinning is needed. >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 >>> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 >>> >>> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx> >>> --- >>> drivers/gpu/drm/gma500/framebuffer.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c >>> index 8b1b6d9..1a11b69 100644 >>> --- a/drivers/gpu/drm/gma500/framebuffer.c >>> +++ b/drivers/gpu/drm/gma500/framebuffer.c >>> @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb) >>> >>> /* Let DRM do its clean up */ >>> drm_framebuffer_cleanup(fb); >>> + >>> + /* Unpin any psb_intel_pipe_set_base() pinning */ >>> + if (r->in_gart > 0) >>> + psb_gtt_unpin(r); >> >> The drm core /should/ have removed the user framebuffer from all active >> users before it calls down into your ->destroy callback. Why can't gma500 >> just pin/unpin on demand when the buffer is actually in use? > > Thanks for the feedback > > DRM core does correctly keep track of the users but the last ref is dropped in > our ->destroy callback and at that point it's still pinned from pipe_set_base(). > So if it's considered too late to unpin the framebuffer in destroy, we never had > it right in the first place. Not a big surprise I guess :) > >> If a pin survives the official use as seen by the drm core (e.g. to keep a >> buffer around until the pageflip completes) you can simply keep an >> additional reference around. > > As I wrote above, that additional reference is not dropped until ->destroy > anyways and we don't have page flipping support and to be honest I haven't even > looked at implementing it yet. So the logical point to release the pin would be > in pipe_set_base() (or am I wrong?) by doing an unpin on the old_fb. Problem is > that when killing X and switching back to fbdev we get no old_fb. > > I might be missing something, so any suggestions are welcome. Dumping our irc discussion for google to index here: I sounds like gma500 code is missing the crtc disabling sequence when X shuts down, i.e. the crtc on->off transition. Then when you switch to fbcon you only get a crtc off->on state transition and so don't see an old fb in set_base, which means that the pin refcount of the old framebuffer is lost. To fix this you can use the special call to crtc_funcs->disable (instead of the default crtc_funcs->dpms(OFF)) in drm_helper_disable_unused_functions. Note that X could also do the vt switch first and then only do the framebuffer destruction, in which case I think your patch here would drop the pin reference twice: Once in set_base since we have an old_fb and once in the framebuffer destroy callback. See intel_crtc_disable in intel_display.c in a v3.6 kernel version for how drm/i915 cleanup up the fb pin reference before we've reworked our modeset code and switched away from the crtc helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
