Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp3248492yba; Mon, 8 Apr 2019 14:31:30 -0700 (PDT) X-Google-Smtp-Source: APXvYqy8oGnTL0ZysfU/Zhf1QIQ8dUOTBvprSgp0DCVi4IMMcD0rOeZVQovBwJtMIQmlolFXXq73 X-Received: by 2002:a63:2747:: with SMTP id n68mr30799158pgn.317.1554759090771; Mon, 08 Apr 2019 14:31:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1554759090; cv=none; d=google.com; s=arc-20160816; b=TizvfHPOTCPZKa/5vkMjmlW7sEddFI7RL9MryUcxeTyIyq4pi21xzOITPiGvZvqsrF YziZfbaQuYEc6aZU9YJoA9ULwf9qhyaznoUebFhBKfPtFkZc7qaDdrVs+sIpRDUQh8Rs P3sQHJcvO756nkZtVrgg9nhQdaG/dlKMDK3UnhHEYqNV8vC8KQVSfOUge6jg2U4rVELr AOVlIL6Yo3zg/tuVHV6z356lJj0uVDH+EmhFVGHkohJOdty77loCLk33H7yl3BSeVONq zAsoCltJHN6/kkJyXknTlymjvqh0y2AFUkstYrCu3pXntmZoc+e+1sJYa+zsSgvcbKRG ZR5w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=fIYuTwVr/1yPlbhCiW4uKPfaJjDMazBgqC4FV6QOgJk=; b=eDk+WVlpyEqP1+6WIOrvlIc3PgyQavzYOQ1Zvf3NZCbXXUf0xBXq1dBgsccQKevEAc hyD8PembE9YWk1S9CgKEQ74U7JXa4cSoztda+uC5r/Qjxr2mgDc55uxv5y+hvKutmsQb 2X4n8IMTguR/jFBvO6SwAdqcJ8O/8Dw5+p4isjc4vuVTHbYlYsUZWvNdHQSxpbKNiIcL BMYAY6t4q/w3sv0WkdIWEY0D9sxCMGptK83/QLK5/tUM25bupbYuVRqbVu/1rcGOVHr4 OI5y5gMi6UbFk/XdxqXDpJC+mJWv1kqEGUubaPy/XUv5I1S6d0FCx+jSUJ9uT5g80yn3 xRWA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=EcsVO5X8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id q197si26857149pgq.411.2019.04.08.14.31.15; Mon, 08 Apr 2019 14:31:30 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=EcsVO5X8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727756AbfDHVEY (ORCPT + 99 others); Mon, 8 Apr 2019 17:04:24 -0400 Received: from mail.kernel.org ([198.145.29.99]:56782 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726349AbfDHVEY (ORCPT ); Mon, 8 Apr 2019 17:04:24 -0400 Received: from mail-qk1-f176.google.com (mail-qk1-f176.google.com [209.85.222.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5844D218EA for ; Mon, 8 Apr 2019 21:04:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1554757462; bh=yzuXJ9W8bPU5MyFLHF0g1NMIYL6IdF2TPars33+T9Wc=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=EcsVO5X8YhjsqsxSGSK6l8nVCTc/9nHbLVvg6lqUfyStHjq7tXRjuPOu0CRvPka25 3a1cxkw3SjQtzM9GHImuGbIVGXyXgOLJF6xFmjUzY8TK88EKw4E2lVlliRSp1+DKOW a4/flaNgUyaJ3xi8I8mrDy5Xr0hakMZGHjUaCtX8= Received: by mail-qk1-f176.google.com with SMTP id g1so8941135qki.5 for ; Mon, 08 Apr 2019 14:04:22 -0700 (PDT) X-Gm-Message-State: APjAAAWPD5lxsUauE9WeeisM0KXZxuaC5JQ6YhqdFWv/PoQUO6aT2Xie yB1/dMaePJT3EDe262pH1CicwUHC8bZJ/RYkSw== X-Received: by 2002:a37:6441:: with SMTP id y62mr24227562qkb.158.1554757461449; Mon, 08 Apr 2019 14:04:21 -0700 (PDT) MIME-Version: 1.0 References: <20190401074730.12241-1-robh@kernel.org> <20190401074730.12241-4-robh@kernel.org> <5efdc3cb-7367-65e1-d1bf-14051db5da10@arm.com> In-Reply-To: <5efdc3cb-7367-65e1-d1bf-14051db5da10@arm.com> From: Rob Herring Date: Mon, 8 Apr 2019 16:04:09 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver To: Steven Price , Tomeu Vizoso Cc: dri-devel , Sean Paul , Maxime Ripard , Neil Armstrong , Will Deacon , "linux-kernel@vger.kernel.org" , David Airlie , Linux IOMMU , Alyssa Rosenzweig , "Marty E . Plummer" , Robin Murphy , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 5, 2019 at 7:30 AM Steven Price wrote: > > On 01/04/2019 08:47, Rob Herring wrote: > > This adds the initial driver for panfrost which supports Arm Mali > > Midgard and Bifrost family of GPUs. Currently, only the T860 and > > T760 Midgard GPUs have been tested. [...] > > + case 0xD3: return "TRANSTAB_BUS_FAULT_LEVEL3"; > > + case 0xD4: return "TRANSTAB_BUS_FAULT_LEVEL4"; > > + case 0xD8: return "ACCESS_FLAG"; > > + } > > + > > + if (panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) { > > There's not a great deal of point in this conditional - you won't get > the below exception codes from hardware which doesn't support it AARCH64 > page tables. I wasn't sure if "UNKNOWN" types could happen or not. > > > + switch (exception_code) { > > + case 0xC9 ... 0xCF: return "PERMISSION_FAULT"; > > + case 0xD9 ... 0xDF: return "ACCESS_FLAG"; > > + case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT"; > > + case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT"; > > + } > > + } > > + > > + return "UNKNOWN"; > > +} > > +static inline int panfrost_model_cmp(struct panfrost_device *pfdev, s32 id) > > +{ > > + s32 match_id = pfdev->features.id; > > + > > + if (match_id & 0xf000) > > + match_id &= 0xf00f; > > I'm puzzled by this, and really not sure if it's going to work out > ignoring the ARCH_MINOR/ARCH_REV fields. But for now there's no real > Bifrost support. That seemed to be enough for kbase to select features/issues. > > + switch (param->param) { > > + case DRM_PANFROST_PARAM_GPU_ID: > > + param->value = pfdev->features.id; > > This is unfortunate naming - this parameter is *not* the GPU_ID. It is > the top half of the GPU_ID which kbase calls the PRODUCT_ID. I can rename it. > I'm also somewhat surprised that you don't need loads of other > properties from the GPU - in particular knowing the number of shader > cores is useful for allocating the right amount of memory for TLS (and > can't be obtained purely from the GPU_ID). We'll add them as userspace needs them. > > +static int > > +panfrost_lookup_bos(struct drm_device *dev, > > + struct drm_file *file_priv, > > + struct drm_panfrost_submit *args, > > + struct panfrost_job *job) > > +{ > > + u32 *handles; > > + int ret = 0; > > + > > + job->bo_count = args->bo_handle_count; > > + > > + if (!job->bo_count) > > + return 0; > > + > > + job->bos = kvmalloc_array(job->bo_count, > > + sizeof(struct drm_gem_object *), > > + GFP_KERNEL | __GFP_ZERO); > > + if (!job->bos) > > If we return here then job->bo_count has been set, but job->bos is > invalid - this means that panfrost_job_cleanup() will then dereference > NULL. Although maybe this is better fixed in panfrost_job_cleanup(). Good catch. The fence arrays have the same problem. As does V3D which we copied. > > + return -ENOMEM; > > + > > + job->implicit_fences = kvmalloc_array(job->bo_count, > > + sizeof(struct dma_fence *), > > + GFP_KERNEL | __GFP_ZERO); > > + if (!job->implicit_fences) > > + return -ENOMEM; > > +static irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) > > +{ > > + struct panfrost_device *pfdev = data; > > + u32 state = gpu_read(pfdev, GPU_INT_STAT); > > + u32 fault_status = gpu_read(pfdev, GPU_FAULT_STATUS); > > + u64 address; > > + bool done = false; > > + > > + if (!state) > > + return IRQ_NONE; > > + > > + if (state & GPU_IRQ_MASK_ERROR) { > > + address = (u64) gpu_read(pfdev, GPU_FAULT_ADDRESS_HI) << 32; > > + address |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); > > + > > + dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n", > > + fault_status & 0xFF, panfrost_exception_name(pfdev, fault_status), > > + address); > > + > > + if (state & GPU_IRQ_MULTIPLE_FAULT) > > + dev_warn(pfdev->dev, "There were multiple GPU faults - some have not been reported\n"); > > + > > + gpu_write(pfdev, GPU_INT_MASK, 0); > > Do you intend to mask off future GPU interrupts? Yes, maybe? If we fault here, then we are going to reset the gpu on timeout which then re-enables interrupts. > > +static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > +{ > > + 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); > > + > > + if (!(status & mask)) > > + continue; > > + > > + job_write(pfdev, JOB_INT_CLEAR, mask); > > + > > + if (status & JOB_INT_MASK_ERR(j)) { > > + job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP); > > + job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0); > > Hard-stopping an already completed job isn't likely to do very much :) > Also you are using the "_0" version which is only valid when "job chain > disambiguation" is present. > > I suspect in this case you should also be signalling the fence? At the > moment you rely on the GPU timeout recovering from the fault. I'll defer to Tomeu who wrote this (IIRC). > > +static void lock_region(struct panfrost_device *pfdev, u32 as_nr, > > + u64 iova, size_t size) > > +{ > > + u8 region_width; > > + u64 region = iova & PAGE_MASK; > > + /* > > + * fls returns (given the ASSERT above): > > But where's the assert? :p > > > + * 1 .. 32 > > + * > > + * 10 + fls(num_pages) > > + * results in the range (11 .. 42) > > + */ > > + > > + size = round_up(size, PAGE_SIZE); > > I'm not sure it matters, but this will break if called on a (small, i.e. > less than a page) region spanning two pages. "region" will be rounded > down to the page (iova & PAGE_MASK), but size will only be rounded up to > the nearest page. This can miss the start of the second page. This is probably redundant. Everything the driver does with memory is in units of pages. Maybe imported buffers could be unaligned. Not sure and we'd probably break in other places if that was the case. > > + /* terminal fault, print info about the fault */ > > + dev_err(pfdev->dev, > > + "Unhandled Page fault in AS%d at VA 0x%016llX\n" > > + "Reason: %s\n" > > + "raw fault status: 0x%X\n" > > + "decoded fault status: %s\n" > > + "exception type 0x%X: %s\n" > > + "access type 0x%X: %s\n" > > + "source id 0x%X\n", > > + i, addr, > > + "TODO", > > + fault_status, > > + (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > > + exception_type, panfrost_exception_name(pfdev, exception_type), > > + access_type, access_type_name(pfdev, fault_status), > > + source_id); > > + > > + mmu_write(pfdev, MMU_INT_CLEAR, mask); > > To fully handle the fault you will need to issue an MMU command (i.e. > call mmu_hw_do_operation()) - obviously after doing something to fix the > cause (e.g. mapping a page or switching the GPU to UNMAPPED mode to kill > off the job). Otherwise you may see that the GPU hangs. Although in > practice at this stage of the driver the generic timeout is probably the > simplest way of handling an MMU fault. Any fault currently is unexpected so all we really care about at this point is getting a message. > > +/** > > + * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs. > > + * > > + * There are currently no values for the flags argument, but it may be > > + * used in a future extension. > > You are probably going to want flags for at least: > > * No execute/No read/No write (good for security, especially with > buffer sharing between processes) > > * Alignment (shader programs have quite strict alignment requirements, > I believe kbase just ensures that the shader memory block doesn't cross > a 16MB boundary which covers most cases). > > * Page fault behaviour (kbase has GROW_ON_GPF) > > * Coherency management Yep, will add them as needed. > One issue that I haven't got to the bottom of is that I can trigger a > lockdep splat: > > -----8<------ > panfrost ffa30000.gpu: js fault, js=1, status=JOB_CONFIG_FAULT, > head=0x0, tail=0x0 > root@debian:~/ddk_panfrost# panfrost ffa30000.gpu: gpu sched timeout, > js=1, status=0x40, head=0x0, tail=0x0, sched_job=12a94ba6 > > ====================================================== > WARNING: possible circular locking dependency detected > 5.0.0+ #32 Not tainted > ------------------------------------------------------ > kworker/1:0/608 is trying to acquire lock: > 89b1e2d8 (&(&js->job_lock)->rlock){-.-.}, at: > dma_fence_remove_callback+0x14/0x50 > > but task is already holding lock: > a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at: > drm_sched_stop+0x24/0x10c > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #1 (&(&sched->job_list_lock)->rlock){-.-.}: > drm_sched_process_job+0x60/0x208 > dma_fence_signal+0x1dc/0x1fc > panfrost_job_irq_handler+0x160/0x194 > __handle_irq_event_percpu+0x80/0x388 > handle_irq_event_percpu+0x24/0x78 > handle_irq_event+0x38/0x5c > handle_fasteoi_irq+0xb4/0x128 > generic_handle_irq+0x18/0x28 > __handle_domain_irq+0xa0/0xb4 > gic_handle_irq+0x4c/0x78 > __irq_svc+0x70/0x98 > arch_cpu_idle+0x20/0x3c > arch_cpu_idle+0x20/0x3c > do_idle+0x11c/0x22c > cpu_startup_entry+0x18/0x20 > start_kernel+0x398/0x420 > > -> #0 (&(&js->job_lock)->rlock){-.-.}: > _raw_spin_lock_irqsave+0x50/0x64 > dma_fence_remove_callback+0x14/0x50 > drm_sched_stop+0x5c/0x10c > panfrost_job_timedout+0xd0/0x180 > drm_sched_job_timedout+0x34/0x5c > process_one_work+0x2ac/0x6a4 > worker_thread+0x28c/0x3fc > kthread+0x13c/0x158 > ret_from_fork+0x14/0x20 > (null) > > other info that might help us debug this: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&(&sched->job_list_lock)->rlock); > lock(&(&js->job_lock)->rlock); > lock(&(&sched->job_list_lock)->rlock); > lock(&(&js->job_lock)->rlock); > > *** DEADLOCK *** > > 3 locks held by kworker/1:0/608: > #0: 9b350627 ((wq_completion)"events"){+.+.}, at: > process_one_work+0x1f8/0x6a4 > #1: a802aa2d ((work_completion)(&(&sched->work_tdr)->work)){+.+.}, at: > process_one_work+0x1f8/0x6a4 > #2: a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at: > drm_sched_stop+0x24/0x10c > > stack backtrace: > CPU: 1 PID: 608 Comm: kworker/1:0 Not tainted 5.0.0+ #32 > Hardware name: Rockchip (Device Tree) > Workqueue: events drm_sched_job_timedout > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x9c/0xd4) > [] (dump_stack) from [] > (print_circular_bug.constprop.15+0x1fc/0x2cc) > [] (print_circular_bug.constprop.15) from [] > (__lock_acquire+0xe5c/0x167c) > [] (__lock_acquire) from [] (lock_acquire+0xc4/0x210) > [] (lock_acquire) from [] > (_raw_spin_lock_irqsave+0x50/0x64) > [] (_raw_spin_lock_irqsave) from [] > (dma_fence_remove_callback+0x14/0x50) > [] (dma_fence_remove_callback) from [] > (drm_sched_stop+0x5c/0x10c) > [] (drm_sched_stop) from [] > (panfrost_job_timedout+0xd0/0x180) > [] (panfrost_job_timedout) from [] > (drm_sched_job_timedout+0x34/0x5c) > [] (drm_sched_job_timedout) from [] > (process_one_work+0x2ac/0x6a4) > [] (process_one_work) from [] > (worker_thread+0x28c/0x3fc) > [] (worker_thread) from [] (kthread+0x13c/0x158) > [] (kthread) from [] (ret_from_fork+0x14/0x20) > Exception stack(0xeebd7fb0 to 0xeebd7ff8) > 7fa0: 00000000 00000000 00000000 > 00000000 > 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 > 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > ----8<---- > > This is with the below simple reproducer: > > ----8<---- > #include > #include > #include > > #include > #include "panfrost_drm.h" > > int main(int argc, char **argv) > { > int fd; > > if (argc == 2) > fd = open(argv[1], O_RDWR); > else > fd = open("/dev/dri/renderD128", O_RDWR); > if (fd == -1) { > perror("Failed to open"); > return 0; > } > > struct drm_panfrost_submit submit = { > .jc = 0, > }; > return ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, &submit); > } > ----8<---- > > Any ideas? I'm not an expert on DRM, so I got somewhat lost! Tomeu? Rob