From patchwork Wed Jun 5 13:06:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrik Jakobsson X-Patchwork-Id: 2669751 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id F31E0DF264 for ; Wed, 5 Jun 2013 13:08:49 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C472AE6512 for ; Wed, 5 Jun 2013 06:08:49 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-la0-f50.google.com (mail-la0-f50.google.com [209.85.215.50]) by gabe.freedesktop.org (Postfix) with ESMTP id CE838E64FB for ; Wed, 5 Jun 2013 06:06:20 -0700 (PDT) Received: by mail-la0-f50.google.com with SMTP id ed20so1412228lab.9 for ; Wed, 05 Jun 2013 06:06:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=7Yg7PYtsi/iPBeCVN3PTDBM6UzHCCIBNyj74RSJQRbI=; b=NQUZ1m9zD3IES0hGH5h4Jx0emx+/ZEyf++Xs7czwvrWxsxbxtBulu3j8IrotkRyizW M3m4ApRCroYNVrJbhltmmmKqFQ5yg1csfcSFe67KBBNZorWLgxKf9TaPyF/l7RHHjiY8 q+VDLlARD5EfgwbCUsNpP1NMDvgqqfxaKZf9hfVIoCfmi0DM02uSC/+dQY7mV4PgyGPd uEW9fC/tYxK222RaQY5XxNccHxn3x9tOJsT/cvebsl2a2lANC2dYo4H+5qeTsu3jdGQT TAy8L0OWpI9/nQs5XP9sUMd5UKTofNwHm5ESptespPfyeoNxCFq7ttziV4GVE2dvaA1H Tj5w== MIME-Version: 1.0 X-Received: by 10.112.157.165 with SMTP id wn5mr14968767lbb.29.1370437579606; Wed, 05 Jun 2013 06:06:19 -0700 (PDT) Received: by 10.112.127.164 with HTTP; Wed, 5 Jun 2013 06:06:19 -0700 (PDT) In-Reply-To: References: <1369591435-1691-1-git-send-email-patrik.r.jakobsson@gmail.com> <20130526190728.GO15743@phenom.ffwll.local> Date: Wed, 5 Jun 2013 15:06:19 +0200 Message-ID: Subject: Re: [PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it From: Patrik Jakobsson To: Daniel Vetter Cc: dri-devel X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org On Sun, May 26, 2013 at 11:00 PM, Daniel Vetter wrote: > On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson > wrote: >> On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter 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 >>>> --- >>>> 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(+) const struct drm_crtc_funcs psb_intel_crtc_funcs = { 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, };