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

[Daniel Vetter]

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.

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