Received: by 2002:a25:31c3:0:0:0:0:0 with SMTP id x186csp829323ybx; Wed, 6 Nov 2019 09:02:29 -0800 (PST) X-Google-Smtp-Source: APXvYqyCODWZtUysoj7lz4K3CRktI8/alticTjY+HsNuAJHMwMtn+0CAilVo5m7fJ084kuy+a9ru X-Received: by 2002:a50:c191:: with SMTP id m17mr3833808edf.259.1573059749392; Wed, 06 Nov 2019 09:02:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1573059749; cv=none; d=google.com; s=arc-20160816; b=Q7QfGBYV8PfzjXr+n/ANG8DH0Q3A4dTJnEe4WD+RlNBgh6rnLZPtaoZwiFYUB4sdvq 79N+hcsbklOeAejKTP7mpoSNeSoZsrc6V08GY9iN+TWy72TMp4lbfbWTgo58js5iF/bp lXFwHjk5TPlo1wz4ANQ7/1/CYku5lG/Ksvj8yfaLjDsaGGVRZYATiCBXPAJ4qG0FJXm8 /CP60V5aQWRhUA0Fi3QBZmxPZ2ZCCkoyI4bxJxJsz58/+FvMsQ7G+b7LGsbrGkWIgwfd 84qR8pD1FUnfseRvaAyMMJfXdOOWCX/FBKZOVsUq+WYWc9l8XWE3Rp94Wn2BXrpeWDY7 zG2Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=gBwyi2vUMFCQ9EewZLonM+Leo+vfm/dcmTg/r5rR65Q=; b=jz7mKE2ZMOysQDHps43BgRnx3bQI+ANBeRX6wglB9Fg20yQUd69TJ4afrMN/d3NTD7 THLzSz3vo62MZkso/5A4SPz2ZERzmzKuoqa2fdW2iPDi4Neeu2uNYBpd4x/2SM1tNSqU jLtihKnFFenHzgOJl3T3RUfW4mFNUNj3YpYgemn4eAYAlJJhMsMdEnaYI2MYECK9zdV6 FfsA8vqk7pSMEsFNIEAryksgXbkrU4pf8FaztSfJXY0K1VsocZiKq2xrkUK95mNLE5Aq 9g8D66yYSv3/YfmxNjNRfLOD0Y8FuqmQy7gHbwt3f6aGewZCN7BpSdZnnb0WqGMrksSG AFvg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=gKTrM77V; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id u24si14213342edl.447.2019.11.06.09.02.03; Wed, 06 Nov 2019 09:02:29 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=gKTrM77V; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732100AbfKFQ7N (ORCPT + 99 others); Wed, 6 Nov 2019 11:59:13 -0500 Received: from mail-ed1-f68.google.com ([209.85.208.68]:40378 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727894AbfKFQ7N (ORCPT ); Wed, 6 Nov 2019 11:59:13 -0500 Received: by mail-ed1-f68.google.com with SMTP id p59so19922314edp.7; Wed, 06 Nov 2019 08:59:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gBwyi2vUMFCQ9EewZLonM+Leo+vfm/dcmTg/r5rR65Q=; b=gKTrM77V4qX8X9qNz87lZHT6mwnmLivmeLFE2de2qflrNk/4mrgkX04n48Isan050a BroS/XWVaIoBDhky1eGK+V5+ai5JQClyjzpVtxCRkgSQLpEwbSqLUc6NXspmxepPQgRb VP0oQ2gklGPuNKrluIIM3JYg7AzWz9733JZz40PVrqFbm3t0y9d68sMPOpCfRC61UJuy SQi1jOz508jT/BwIq2ZRW/+vgiIQV5UWCw+sY2TKiDyi5/VLhttTJZ+FKpLXw3nchLfL BGhoxx4Q65ewLeaZmVeNBGg9dY3cV/2Yb72fFpNfsLWyoyWTPlDYAXZgOK6a1XFdmUIr oLRA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gBwyi2vUMFCQ9EewZLonM+Leo+vfm/dcmTg/r5rR65Q=; b=Gfvz/AjGdrMuIcvbYh3RMBJKZnhwx8Q8gwbUZkeBFtwW/qUhIbZN9aTCTqCJu5YXS5 gAdakheywFGMuuwHBXD7d3nWkvKMrQPOvnvfrLMcW20V3cnvq9UnGH4DidyTIJUl2IFp byUcdrcxHcvqvr496rNG+4v9zkcR3meqI3cshJAboo0Lbmsp8833X2WkOP2PvVsKn8mW 4WysV3quny2i1p7kaAMx38bPPTSooDwUO7Zbj5Di2BQZjdF/0xJhcSHvVQE3WkG7CBAF T7boN/REC4oUCHwo26/DC4BiqyWK3ZX4FO9x4CAD29FKAox1OgkkikD8eGURbD6v+lzb dzsA== X-Gm-Message-State: APjAAAXe9buJgk97Zc6J2VxHvif15L+2s16Y+iWvtFRqfrKAHrU0NxoJ EfZ2DbqtU8u2oNJTEBjlVZ9WFMaT0AP5IS0FqHE= X-Received: by 2002:a50:a697:: with SMTP id e23mr3871635edc.264.1573059550755; Wed, 06 Nov 2019 08:59:10 -0800 (PST) MIME-Version: 1.0 References: <20191105000129.GA6536@onstation.org> <20191105100804.GA9492@onstation.org> <20191106091335.GA16729@onstation.org> In-Reply-To: From: Rob Clark Date: Wed, 6 Nov 2019 08:58:59 -0800 Message-ID: Subject: Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes To: Jeffrey Hugo Cc: Brian Masney , Rob Clark , freedreno , Linux Kernel Mailing List , dri-devel , linux-arm-msm , Sean Paul Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo wrote: > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark wrote: > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney wrote: > > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote: > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney wrote: > > > > > The 'pp done time out' errors go away if I revert the following three > > > > > commits: > > > > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support") > > > > > d934a712c5e6 ("drm/msm: add atomic traces") > > > > > 2d99ced787e3 ("drm/msm: async commit support") > > > > > > > > > > I reverted the first one to fix a compiler error, and the second one so > > > > > that the last patch can be reverted without any merge conflicts. > > > > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I tried to use > > > > > crtc_flush_all() in mdp5_flush_commit() and the contents of the frame > > > > > buffer dance around the screen like its out of sync. I renamed > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed the static > > > > > declaration. Here's the relevant part of what I tried: > > > > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > > > > > @@ -171,7 +171,15 @@ static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st > > > > > > > > > > static void mdp5_flush_commit(struct msm_kms *kms, unsigned crtc_mask) > > > > > { > > > > > - /* TODO */ > > > > > + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > > > > > + struct drm_crtc *crtc; > > > > > + > > > > > + for_each_crtc_mask(mdp5_kms->dev, crtc, crtc_mask) { > > > > > + if (!crtc->state->active) > > > > > + continue; > > > > > + > > > > > + mdp5_crtc_flush_all(crtc); > > > > > + } > > > > > } > > > > > > > > > > Any tips would be appreciated. > > > > > > > > > > > > I think this is along the lines of what we need to enable async commit > > > > for mdp5 (but also removing the flush from the atomic-commit path).. > > > > the principle behind the async commit is to do all the atomic state > > > > commit normally, but defer writing the flush bits. This way, if you > > > > get another async update before the next vblank, you just apply it > > > > immediately instead of waiting for vblank. > > > > > > > > But I guess you are on a command mode panel, if I remember? Which is > > > > a case I didn't have a way to test. And I'm not entirely about how > > > > kms_funcs->vsync_time() should be implemented for cmd mode panels. > > > > > > Yes, this is a command-mode panel and there's no hardware frame counter > > > available. The key to getting the display working on this phone was this > > > patch: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2bab52af6fe68c43b327a57e5ce5fc10eefdfadf > > > > > > > That all said, I think we should first fix what is broken, before > > > > worrying about extending async commit support to mdp5.. which > > > > shouldn't hit the async==true path, due to not implementing > > > > kms_funcs->vsync_time(). > > > > > > > > What I think is going on is that, in the cmd mode case, > > > > mdp5_wait_flush() (indirectly) calls mdp5_crtc_wait_for_pp_done(), > > > > which waits for a pp-done irq regardless of whether there is a flush > > > > in progress. Since there is no flush pending, the irq never comes. > > > > But the expectation is that kms_funcs->wait_flush() returns > > > > immediately if there is nothing to wait for. > > > > > > I don't think that's happening in this case. I added some pr_info() > > > statements to request_pp_done_pending() and mdp5_crtc_pp_done_irq(). > > > Here's the first two sets of messages that appear in dmesg: > > > > > > [ 14.018907] msm fd900000.mdss: pp done time out, lm=0 > > > [ 14.018993] request_pp_done_pending: HERE > > > [ 14.074208] mdp5_crtc_pp_done_irq: HERE > > > [ 14.074368] Console: switching to colour frame buffer device 135x120 > > > [ 14.138938] msm fd900000.mdss: pp done time out, lm=0 > > > [ 14.139021] request_pp_done_pending: HERE > > > [ 14.158097] mdp5_crtc_pp_done_irq: HERE > > > > > > The messages go on like this with the same pattern. > > > > > > I tried two different changes: > > > > > > 1) I moved the request_pp_done_pending() and corresponding if statement > > > from mdp5_crtc_atomic_flush() and into mdp5_crtc_atomic_begin(). > > > > > > 2) I increased the timeout in wait_for_completion_timeout() by several > > > increments; all the way to 5 seconds. > > > > increasing the timeout won't help, because the pp-done irq has already > > come at the point where we wait for it.. > > > > maybe the easy thing is just add mdp5_crtc->needs_pp, set to true > > before requesting, and false when we get the irq.. and then > > mdp5_crtc_wait_for_pp_done() just returns if needs_pp==false.. > > On the otherhand, what about trying to make command mode panels > resemble video mode panels slightly? Video mode panels have a vsync > counter in hardware, which is missing from command mode - however it > seems like the driver/drm framework would prefer such a counter. > Would it be a reasonable idea to make a software counter, and just > increment it every time the pp_done irq is triggered? > > I'm just thinking that we'll avoid issues long term by trying to make > the code common, rather than diverging it for the two modes. > *possibly*, but I think we want to account somehow periods where display is not updated. fwiw, it isn't that uncommon for userspace to use vblanks to "keep time" (drive animations for desktop switch, window maximize/unmaximize, etc).. it could be a surprise when "vblank" is not periodic. BR, -R