[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it

[Patrik Jakobsson]

On Sun, May 26, 2013 at 11:00 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson
> <patrik.r.jakobsson at gmail.com> wrote:
>> On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter <daniel at 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 at 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 = {
-- 
1.8.1.2