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@gmail.com --- 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); + /* We are no longer using the resource in GEM */ drm_gem_object_unreference_unlocked(&r->gem); kfree(fb);
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@gmail.com
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?
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.
Cheers, Daniel
- /* We are no longer using the resource in GEM */ drm_gem_object_unreference_unlocked(&r->gem); kfree(fb);
-- 1.8.1.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter daniel@ffwll.ch 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@gmail.com
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.
Thanks Patrik
On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson patrik.r.jakobsson@gmail.com wrote:
On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter daniel@ffwll.ch 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@gmail.com
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
On Sun, May 26, 2013 at 11:00 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson patrik.r.jakobsson@gmail.com wrote:
On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter daniel@ffwll.ch 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@gmail.com
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 = {
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Patrik Jakobsson