From: Brian Starkey <Brian.Starkey@arm.com> To: Daniel Vetter <email@example.com> Cc: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>, Liviu Dudau <Liviu.Dudau@arm.com>, "james qian wang (Arm Technology China)" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "Julien Yin (Arm Technology China)" <Julien.Yin@arm.com>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@arm.com>, Ayan Halder <Ayan.Halder@arm.com>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, nd <email@example.com> Subject: Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Date: Fri, 2 Aug 2019 10:09:05 +0000 [thread overview] Message-ID: <20190802100904.blnusnieti3pxgxu@DESKTOP-E1NTVVP.localdomain> (raw) In-Reply-To: <CAKMK7uH38rxyTyuYRGJ6NBejyUxQ6Qvr1CdjH2kpXiq+3-=t8Q@mail.gmail.com> Hi Daniel, On Fri, Aug 02, 2019 at 11:45:13AM +0200, Daniel Vetter wrote: > On Fri, Aug 2, 2019 at 11:43 AM Daniel Vetter <firstname.lastname@example.org> wrote: > > > > On Fri, Aug 2, 2019 at 11:29 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > > > > > Hi Lowry, > > > > > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > > > Hi Brian, > > > > > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > > > Hi Lowry, > > > > > > > > > > Thanks for this cleanup. > > > > > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > > > During it signals the completion of a writeback job, after releasing > > > > > > the out_fence, we'd clear the pointer. > > > > > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <email@example.com> > > > > > > --- > > > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > > > index ff138b6..43d9e3b 100644 > > > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > > > if (job->fb) > > > > > > drm_framebuffer_put(job->fb); > > > > > > > > > > > > + if (job->out_fence) > > > > > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > > > here, if it's not already signaled. Otherwise, if there's someone > > > > > waiting (which there shouldn't be), they're going to be waiting a very > > > > > long time :-) > > > > > > > > > > Thanks, > > > > > -Brian > > > > > > > > > Here it happened at atomic_check failed and test only commit. For both > > > > cases, the commit has been dropped and it's only a clean up. So here better > > > > not be treated as an error case:) > > > > > > If anyone else has a reference on the fence, then IMO it absolutely is > > > an error to reach this point without the fence being signaled - > > > because it means that the fence will never be signaled. > > > > > > I don't think the API gives you a way to check if this is the last > > > reference, so it's safest to just make sure the fence is signalled > > > before dropping the reference. > > > > > > It just feels wrong to me to have the possibility of a dangling fence > > > which is never going to get signalled; and it's an easy defensive step > > > to make sure it can never happen. > > > > > > I know it _shouldn't_ happen, but we often put in handling for cases > > > which shouldn't happen, because they frequently do happen :-) > > > > We're not as paranoid with the vblank fences either, so not sure why > > we need to be this paranoid with writeback fences. If your driver > > grabs anything from the atomic state in ->atomic_check it's buggy > > anyway. > > > > If you want to fix this properly I think we need to move the call to > > prepare_signalling() in between atomic_check and atomic_commit. Then I > > think it makes sense to also force-complete the fence on error ... Well, fair enough. I'm struggling with "that's too paranoid" vs "fix it properly" though? Is it a "problem" worth fixing or not? It seems natural to me to do the fence cleanup in the cleanup function for the object which owns the fence. > > > > > > Since for userspace, it should have been failed or a test only case, so > > > > writebace fence should not be signaled. > > > > > > It's not only userspace that can wait on fences (and in fact this > > > fence will never even reach userspace if the commit fails), the driver > > > may have taken a copy to use for "something". > > I forgot to add: you can check this by looking at the fence reference > count. A WARN_ON if that's more than 1 on cleanup (but also for the > out fences) could be a nice addition. Do we really want to be looking at the fence internals directly like that? Cheers, -Brian > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2019-08-02 10:09 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-31 11:04 [PATCH v1 0/2] Free the writeback_job when it with an empty fb Lowry Li (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) 2019-07-31 13:03 ` Liviu Dudau 2019-09-23 7:24 ` [v1,1/2] " james qian wang (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 2019-07-31 13:15 ` Liviu Dudau [not found] ` <20190801063055.GA17887@firstname.lastname@example.org> 2019-08-01 9:58 ` Liviu Dudau 2019-07-31 13:20 ` Brian Starkey [not found] ` <20190801063351.GB17887@email@example.com> 2019-08-02 9:29 ` Brian Starkey 2019-08-02 9:43 ` Daniel Vetter 2019-08-02 9:45 ` Daniel Vetter 2019-08-02 10:09 ` Brian Starkey [this message] 2019-08-02 14:06 ` Daniel Vetter 2019-08-02 10:09 ` james qian wang (Arm Technology China) 2019-08-05 13:10 ` Brian Starkey 2019-09-23 7:25 ` [v1,2/2] " james qian wang (Arm Technology China)
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190802100904.blnusnieti3pxgxu@DESKTOP-E1NTVVP.localdomain \ --firstname.lastname@example.org \ --cc=Ayan.Halder@arm.com \ --cc=Jonathan.Chai@arm.com \ --cc=Julien.Yin@arm.com \ --cc=Liviu.Dudau@arm.com \ --cc=Lowry.Li@arm.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).