linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brian Starkey <Brian.Starkey@arm.com>
To: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>,
	"james qian wang (Arm Technology China)"
	<james.qian.wang@arm.com>,
	"maarten.lankhorst@linux.intel.com" 
	<maarten.lankhorst@linux.intel.com>,
	"seanpaul@chromium.org" <seanpaul@chromium.org>,
	"airlied@linux.ie" <airlied@linux.ie>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"Julien Yin (Arm Technology China)" <Julien.Yin@arm.com>,
	"maxime.ripard@bootlin.com" <maxime.ripard@bootlin.com>,
	"eric@anholt.net" <eric@anholt.net>,
	"kieran.bingham+renesas@ideasonboard.com" 
	<kieran.bingham+renesas@ideasonboard.com>,
	"sean@poorly.run" <sean@poorly.run>,
	"laurent.pinchart@ideasonboard.com" 
	<laurent.pinchart@ideasonboard.com>,
	"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@arm.com>,
	Ayan Halder <Ayan.Halder@arm.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>, nd <nd@arm.com>
Subject: Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled
Date: Fri, 2 Aug 2019 09:29:20 +0000	[thread overview]
Message-ID: <20190802092920.4la5cwrltv2m6dke@DESKTOP-E1NTVVP.localdomain> (raw)
In-Reply-To: <20190801063351.GB17887@lowry.li@arm.com>

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) <lowry.li@arm.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 :-)

> 
> 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".

Cheers,
-Brian

> 
> Best regards,
> Lowry
> > > +		dma_fence_put(job->out_fence);
> > > +
> > >  	kfree(job);
> > >  }
> 
> -- 
> Regards,
> Lowry

  parent reply	other threads:[~2019-08-02  9:29 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@lowry.li@arm.com>
2019-08-01  9:58       ` Liviu Dudau
2019-07-31 13:20   ` Brian Starkey
     [not found]     ` <20190801063351.GB17887@lowry.li@arm.com>
2019-08-02  9:29       ` Brian Starkey [this message]
2019-08-02  9:43         ` Daniel Vetter
2019-08-02  9:45           ` Daniel Vetter
2019-08-02 10:09             ` Brian Starkey
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=20190802092920.4la5cwrltv2m6dke@DESKTOP-E1NTVVP.localdomain \
    --to=brian.starkey@arm.com \
    --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 \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=eric@anholt.net \
    --cc=james.qian.wang@arm.com \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=maxime.ripard@bootlin.com \
    --cc=nd@arm.com \
    --cc=sean@poorly.run \
    --cc=seanpaul@chromium.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).