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=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 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 B83C9C2B9F4 for ; Fri, 25 Jun 2021 14:37:59 +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 827A261963 for ; Fri, 25 Jun 2021 14:37:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 827A261963 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.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 D4A4F6EDEB; Fri, 25 Jun 2021 14:37:58 +0000 (UTC) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by gabe.freedesktop.org (Postfix) with ESMTPS id BF5606EDEB for ; Fri, 25 Jun 2021 14:37:57 +0000 (UTC) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 485BA1F44761; Fri, 25 Jun 2021 15:37:56 +0100 (BST) Date: Fri, 25 Jun 2021 16:37:53 +0200 From: Boris Brezillon To: Alyssa Rosenzweig Subject: Re: [PATCH v3 08/15] drm/panfrost: Use a threaded IRQ for job interrupts Message-ID: <20210625163722.3d44a7c7@collabora.com> In-Reply-To: References: <20210625133327.2598825-1-boris.brezillon@collabora.com> <20210625133327.2598825-9-boris.brezillon@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Tomeu Vizoso , dri-devel@lists.freedesktop.org, Steven Price , Rob Herring , Alyssa Rosenzweig , Robin Murphy Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Fri, 25 Jun 2021 09:47:59 -0400 Alyssa Rosenzweig wrote: > A-b, but could you explain the context? Thanks The rational behind this change is the complexity added to the interrupt handler in patch 15. That means we might spend more time in interrupt context after that patch and block other things on the system while we dequeue job irqs. Moving things to a thread also helps performances when the GPU gets faster as executing jobs than the CPU at queueing them. In that case we keep switching back-and-forth between interrupt and non-interrupt context which has a cost. One drawback is increased latency when receiving job events and the thread is idle, since you need to wake up the thread in that case. > > On Fri, Jun 25, 2021 at 03:33:20PM +0200, Boris Brezillon wrote: > > This should avoid switching to interrupt context when the GPU is under > > heavy use. > > > > v3: > > * Don't take the job_lock in panfrost_job_handle_irq() > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/gpu/drm/panfrost/panfrost_job.c | 53 ++++++++++++++++++------- > > 1 file changed, 38 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > > index be8f68f63974..e0c479e67304 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > > @@ -470,19 +470,12 @@ static const struct drm_sched_backend_ops panfrost_sched_ops = { > > .free_job = panfrost_job_free > > }; > > > > -static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > +static void panfrost_job_handle_irq(struct panfrost_device *pfdev, u32 status) > > { > > - struct panfrost_device *pfdev = data; > > - u32 status = job_read(pfdev, JOB_INT_STAT); > > int j; > > > > dev_dbg(pfdev->dev, "jobslot irq status=%x\n", status); > > > > - if (!status) > > - return IRQ_NONE; > > - > > - pm_runtime_mark_last_busy(pfdev->dev); > > - > > for (j = 0; status; j++) { > > u32 mask = MK_JS_MASK(j); > > > > @@ -519,7 +512,6 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > if (status & JOB_INT_MASK_DONE(j)) { > > struct panfrost_job *job; > > > > - spin_lock(&pfdev->js->job_lock); > > job = pfdev->jobs[j]; > > /* Only NULL if job timeout occurred */ > > if (job) { > > @@ -531,21 +523,49 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > dma_fence_signal_locked(job->done_fence); > > pm_runtime_put_autosuspend(pfdev->dev); > > } > > - spin_unlock(&pfdev->js->job_lock); > > } > > > > status &= ~mask; > > } > > +} > > > > +static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data) > > +{ > > + struct panfrost_device *pfdev = data; > > + u32 status = job_read(pfdev, JOB_INT_RAWSTAT); > > + > > + while (status) { > > + pm_runtime_mark_last_busy(pfdev->dev); > > + > > + spin_lock(&pfdev->js->job_lock); > > + panfrost_job_handle_irq(pfdev, status); > > + spin_unlock(&pfdev->js->job_lock); > > + status = job_read(pfdev, JOB_INT_RAWSTAT); > > + } > > + > > + job_write(pfdev, JOB_INT_MASK, > > + GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | > > + GENMASK(NUM_JOB_SLOTS - 1, 0)); > > return IRQ_HANDLED; > > } > > > > +static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > +{ > > + struct panfrost_device *pfdev = data; > > + u32 status = job_read(pfdev, JOB_INT_STAT); > > + > > + if (!status) > > + return IRQ_NONE; > > + > > + job_write(pfdev, JOB_INT_MASK, 0); > > + return IRQ_WAKE_THREAD; > > +} > > + > > static void panfrost_reset(struct work_struct *work) > > { > > struct panfrost_device *pfdev = container_of(work, > > struct panfrost_device, > > reset.work); > > - unsigned long flags; > > unsigned int i; > > bool cookie; > > > > @@ -575,7 +595,7 @@ static void panfrost_reset(struct work_struct *work) > > /* All timers have been stopped, we can safely reset the pending state. */ > > atomic_set(&pfdev->reset.pending, 0); > > > > - spin_lock_irqsave(&pfdev->js->job_lock, flags); > > + spin_lock(&pfdev->js->job_lock); > > for (i = 0; i < NUM_JOB_SLOTS; i++) { > > if (pfdev->jobs[i]) { > > pm_runtime_put_noidle(pfdev->dev); > > @@ -583,7 +603,7 @@ static void panfrost_reset(struct work_struct *work) > > pfdev->jobs[i] = NULL; > > } > > } > > - spin_unlock_irqrestore(&pfdev->js->job_lock, flags); > > + spin_unlock(&pfdev->js->job_lock); > > > > panfrost_device_reset(pfdev); > > > > @@ -610,8 +630,11 @@ int panfrost_job_init(struct panfrost_device *pfdev) > > if (irq <= 0) > > return -ENODEV; > > > > - ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler, > > - IRQF_SHARED, KBUILD_MODNAME "-job", pfdev); > > + ret = devm_request_threaded_irq(pfdev->dev, irq, > > + panfrost_job_irq_handler, > > + panfrost_job_irq_handler_thread, > > + IRQF_SHARED, KBUILD_MODNAME "-job", > > + pfdev); > > if (ret) { > > dev_err(pfdev->dev, "failed to request job irq"); > > return ret; > > -- > > 2.31.1 > >