From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1AE97C4363D for ; Fri, 2 Oct 2020 11:08:36 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BED5C206B8 for ; Fri, 2 Oct 2020 11:08:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BED5C206B8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 40E506E92B; Fri, 2 Oct 2020 11:08:34 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EA9F6E92A for ; Fri, 2 Oct 2020 11:08:32 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DA5171063; Fri, 2 Oct 2020 04:08:31 -0700 (PDT) Received: from [192.168.1.179] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E1F3F3F73B; Fri, 2 Oct 2020 04:08:30 -0700 (PDT) Subject: Re: [PATCH v2] drm/panfrost: Fix job timeout handling To: Boris Brezillon , Rob Herring , Tomeu Vizoso , Alyssa Rosenzweig , Robin Murphy References: <20201002071032.1225267-1-boris.brezillon@collabora.com> From: Steven Price Message-ID: <3cd377c1-4456-eb60-8da5-d44e398697b7@arm.com> Date: Fri, 2 Oct 2020 12:08:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201002071032.1225267-1-boris.brezillon@collabora.com> Content-Language: en-GB X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stable@vger.kernel.org, dri-devel@lists.freedesktop.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 02/10/2020 08:10, Boris Brezillon wrote: > If more than two jobs end up timeout-ing concurrently, only one of them > (the one attached to the scheduler acquiring the lock) is fully handled. > The other one remains in a dangling state where it's no longer part of > the scheduling queue, but still blocks something in scheduler, leading > to repetitive timeouts when new jobs are queued. > > Let's make sure all bad jobs are properly handled by the thread > acquiring the lock. > > v2: > - Fix the subject prefix > - Stop the scheduler before returning from panfrost_job_timedout() > - Call cancel_delayed_work_sync() after drm_sched_stop() to make sure > no timeout handlers are in flight when we reset the GPU (Steven Price) > - Make sure we release the reset lock before restarting the > schedulers (Steven Price) > > Signed-off-by: Boris Brezillon > Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") > Cc: LGTM! Reviewed-by: Steven Price > --- > drivers/gpu/drm/panfrost/panfrost_job.c | 64 +++++++++++++++++++++---- > 1 file changed, 55 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 30e7b7196dab..6e4bfb938fab 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -25,7 +25,8 @@ > > struct panfrost_queue_state { > struct drm_gpu_scheduler sched; > - > + bool stopped; > + struct mutex lock; > u64 fence_context; > u64 emit_seqno; > }; > @@ -369,6 +370,24 @@ void panfrost_job_enable_interrupts(struct panfrost_device *pfdev) > job_write(pfdev, JOB_INT_MASK, irq_mask); > } > > +static bool panfrost_scheduler_stop(struct panfrost_queue_state *queue, > + struct drm_sched_job *bad) > +{ > + bool stopped = false; > + > + mutex_lock(&queue->lock); > + if (!queue->stopped) { > + drm_sched_stop(&queue->sched, bad); > + if (bad) > + drm_sched_increase_karma(bad); > + queue->stopped = true; > + stopped = true; > + } > + mutex_unlock(&queue->lock); > + > + return stopped; > +} > + > static void panfrost_job_timedout(struct drm_sched_job *sched_job) > { > struct panfrost_job *job = to_panfrost_job(sched_job); > @@ -392,19 +411,41 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) > job_read(pfdev, JS_TAIL_LO(js)), > sched_job); > > + /* Scheduler is already stopped, nothing to do. */ > + if (!panfrost_scheduler_stop(&pfdev->js->queue[js], sched_job)) > + return; > + > if (!mutex_trylock(&pfdev->reset_lock)) > return; > > + mutex_lock(&pfdev->sched_lock); > for (i = 0; i < NUM_JOB_SLOTS; i++) { > struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched; > > - drm_sched_stop(sched, sched_job); > - if (js != i) > - /* Ensure any timeouts on other slots have finished */ > + /* > + * If the queue is still active, make sure we wait for any > + * pending timeouts. > + */ > + if (!pfdev->js->queue[i].stopped) > cancel_delayed_work_sync(&sched->work_tdr); > - } > > - drm_sched_increase_karma(sched_job); > + /* > + * If the scheduler was not already stopped, there's a tiny > + * chance a timeout has expired just before we stopped it, and > + * drm_sched_stop() does not flush pending works. Let's flush > + * them now so the timeout handler doesn't get called in the > + * middle of a reset. > + */ > + if (panfrost_scheduler_stop(&pfdev->js->queue[i], NULL)) > + cancel_delayed_work_sync(&sched->work_tdr); > + > + /* > + * Now that we cancelled the pending timeouts, we can safely > + * reset the stopped state. > + */ > + pfdev->js->queue[i].stopped = false; > + } > + mutex_unlock(&pfdev->sched_lock); > > spin_lock_irqsave(&pfdev->js->job_lock, flags); > for (i = 0; i < NUM_JOB_SLOTS; i++) { > @@ -421,11 +462,11 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) > for (i = 0; i < NUM_JOB_SLOTS; i++) > drm_sched_resubmit_jobs(&pfdev->js->queue[i].sched); > > + mutex_unlock(&pfdev->reset_lock); > + > /* restart scheduler after GPU is usable again */ > for (i = 0; i < NUM_JOB_SLOTS; i++) > drm_sched_start(&pfdev->js->queue[i].sched, true); > - > - mutex_unlock(&pfdev->reset_lock); > } > > static const struct drm_sched_backend_ops panfrost_sched_ops = { > @@ -558,6 +599,7 @@ int panfrost_job_open(struct panfrost_file_priv *panfrost_priv) > int ret, i; > > for (i = 0; i < NUM_JOB_SLOTS; i++) { > + mutex_init(&js->queue[i].lock); > sched = &js->queue[i].sched; > ret = drm_sched_entity_init(&panfrost_priv->sched_entity[i], > DRM_SCHED_PRIORITY_NORMAL, &sched, > @@ -570,10 +612,14 @@ int panfrost_job_open(struct panfrost_file_priv *panfrost_priv) > > void panfrost_job_close(struct panfrost_file_priv *panfrost_priv) > { > + struct panfrost_device *pfdev = panfrost_priv->pfdev; > + struct panfrost_job_slot *js = pfdev->js; > int i; > > - for (i = 0; i < NUM_JOB_SLOTS; i++) > + for (i = 0; i < NUM_JOB_SLOTS; i++) { > drm_sched_entity_destroy(&panfrost_priv->sched_entity[i]); > + mutex_destroy(&js->queue[i].lock); > + } > } > > int panfrost_job_is_idle(struct panfrost_device *pfdev) > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel