Re: [PATCH v5 04/21] gpu: host1x: Remove cancelled waiters immediately

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 13, 2021 at 12:20:38AM +0200, Mikko Perttunen wrote:
> On 1/13/21 12:07 AM, Dmitry Osipenko wrote:
> > 11.01.2021 16:00, Mikko Perttunen пишет:
> > > -void host1x_intr_put_ref(struct host1x *host, unsigned int id, void *ref)
> > > +void host1x_intr_put_ref(struct host1x *host, unsigned int id, void *ref,
> > > +			 bool flush)
> > >   {
> > >   	struct host1x_waitlist *waiter = ref;
> > >   	struct host1x_syncpt *syncpt;
> > > -	while (atomic_cmpxchg(&waiter->state, WLS_PENDING, WLS_CANCELLED) ==
> > > -	       WLS_REMOVED)
> > > -		schedule();
> > > +	atomic_cmpxchg(&waiter->state, WLS_PENDING, WLS_CANCELLED);
> > >   	syncpt = host->syncpt + id;
> > > -	(void)process_wait_list(host, syncpt,
> > > -				host1x_syncpt_load(host->syncpt + id));
> > > +
> > > +	spin_lock(&syncpt->intr.lock);
> > > +	if (atomic_cmpxchg(&waiter->state, WLS_CANCELLED, WLS_HANDLED) ==
> > > +	    WLS_CANCELLED) {
> > > +		list_del(&waiter->list);
> > > +		kref_put(&waiter->refcount, waiter_release);
> > > +	}
> > > +	spin_unlock(&syncpt->intr.lock);
> > > +
> > > +	if (flush) {
> > > +		/* Wait until any concurrently executing handler has finished. */
> > > +		while (atomic_read(&waiter->state) != WLS_HANDLED)
> > > +			cpu_relax();
> > > +	}
> > 
> > A busy-loop shouldn't be used in kernel unless there is a very good
> > reason. The wait_event() should be used instead.
> > 
> > But please don't hurry to update this patch, we may need or want to
> > retire the host1x-waiter and then these all waiter-related patches won't
> > be needed.
> > 
> 
> Yes, we should improve the intr code to remove all this complexity. But
> let's merge this first to get a functional baseline and do larger design
> changes in follow-up patches.

I agree, there's no reason to hold back any interim improvements. But I
do agree with Dmitry's argument about the busy loop. Prior to this, the
code used schedule() to let the CPU run other code while waiting for the
waiter to change state. Is there any reason why we can't keep schedule()
here?

Thierry

Attachment: signature.asc
Description: PGP signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux