On Sun, May 26, 2013 at 11:00 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > 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. Adding the disable callback when we have a fb != NULL solves the problem. It also looks like we never turn of the cursor. Is this a good place for that as well? > 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. I had a check for that (gtt->in_gart > 0) but still, doing it in ->disable solved that as well. > 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. Here's what it currently looks like: --- drivers/gpu/drm/gma500/psb_intel_display.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c index 6e8f42b..a16b1a8 100644 --- a/drivers/gpu/drm/gma500/psb_intel_display.c +++ b/drivers/gpu/drm/gma500/psb_intel_display.c @@ -1150,6 +1150,18 @@ static void psb_intel_crtc_destroy(struct drm_crtc *crtc) kfree(psb_intel_crtc); } +static void psb_intel_crtc_disable(struct drm_crtc *crtc) { + struct gtt_range *gt; + struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; + + crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF); + + if (crtc->fb) { + gt = to_psb_fb(crtc->fb)->gtt; + psb_gtt_unpin(gt); + } +} + const struct drm_crtc_helper_funcs psb_intel_helper_funcs = { .dpms = psb_intel_crtc_dpms, .mode_fixup = psb_intel_crtc_mode_fixup, @@ -1157,6 +1169,7 @@ const struct drm_crtc_helper_funcs psb_intel_helper_funcs = { .mode_set_base = psb_intel_pipe_set_base, .prepare = psb_intel_crtc_prepare, .commit = psb_intel_crtc_commit, + .disable = psb_intel_crtc_disable, }; const struct drm_crtc_funcs psb_intel_crtc_funcs = { -- 1.8.1.2 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
