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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E58F6C433F5 for ; Wed, 11 May 2022 19:05:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346565AbiEKTFS (ORCPT ); Wed, 11 May 2022 15:05:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57900 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236747AbiEKTFP (ORCPT ); Wed, 11 May 2022 15:05:15 -0400 Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BCDB661604 for ; Wed, 11 May 2022 12:05:13 -0700 (PDT) Received: by mail-ed1-x52f.google.com with SMTP id a21so3693475edb.1 for ; Wed, 11 May 2022 12:05:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=4K/BqknQQ3B81Ri9IMPLTLrHNMzicKZVVfstqbcQoXw=; b=A/KCQGg5ugLO69SideOJEv3eS/RyBeILjrfEq1trEjYp4cb9RZCmIZVFqxyI7WsooG XsWzCAMH5B+G8qNASIFfM8v3cZTbZl3x5TWJGNd90LgqR8xJipz7OP0NHwqKYzKNE3I7 MVIOk5NK+bWC0jwrLMNufBwoueLjBB6jdZuiE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=4K/BqknQQ3B81Ri9IMPLTLrHNMzicKZVVfstqbcQoXw=; b=4V3IJG+cueFvWR/UtLuSufTojPkagDyHSjMi23iBqOjXiUIyXj6I12n4BuZYA8dLgH Jpa+bxaSHjhYAocH9C97G2fmASusMxb1a6ALxl0eHBFyu5nAdO3ydhbMUfE+x8ApE9o+ 5wy0u6LW/eZG3z6e/uodH+H/OdHL52mJFSeBxVzRYiRj4cLe6HTHIBagNznt+hnvusPP di92PTOTIxk6j6Fqi+vzVkXVp58Cky4XwysJHXLF9anacksMXXX97fUqUFph7zHGsxz0 Za+Or32LZLkKWRBhDM+1/itEzzdqozlX4xH0NBN8QnTHLiNFqM9KbwPFXHhHXbhQvsIU gOtA== X-Gm-Message-State: AOAM530VxrRTAARq5E2sPaV/cD6BNzzZVfuX1DkgcHWV3GqGRODtFRUr jvmS5meoFFgB9dja3O6nw2zQ7A== X-Google-Smtp-Source: ABdhPJxzbQWruH8VN9oOr7p/K2/W+DT07g4nY6eAZpa/3zcSLOTXAgDkDgKjcZvZEO42b/8vTn+Ifg== X-Received: by 2002:a05:6402:278d:b0:42a:2dc0:744f with SMTP id b13-20020a056402278d00b0042a2dc0744fmr1951646ede.226.1652295912301; Wed, 11 May 2022 12:05:12 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id i26-20020a1709063c5a00b006fa9384a0b5sm1259045ejg.61.2022.05.11.12.05.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 May 2022 12:05:11 -0700 (PDT) Date: Wed, 11 May 2022 21:05:09 +0200 From: Daniel Vetter To: Dmitry Osipenko Cc: Christian =?iso-8859-1?Q?K=F6nig?= , Thomas Zimmermann , Daniel Vetter , Daniel Stone , David Airlie , Gerd Hoffmann , Gurchetan Singh , Chia-I Wu , Daniel Almeida , Gert Wollny , Gustavo Padovan , Tomeu Vizoso , Maarten Lankhorst , Maxime Ripard , Rob Herring , Steven Price , Alyssa Rosenzweig , Rob Clark , Emil Velikov , Robin Murphy , Dmitry Osipenko , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v4 10/15] drm/shmem-helper: Take reservation lock instead of drm_gem_shmem locks Message-ID: Mail-Followup-To: Dmitry Osipenko , Christian =?iso-8859-1?Q?K=F6nig?= , Thomas Zimmermann , Daniel Stone , David Airlie , Gerd Hoffmann , Gurchetan Singh , Chia-I Wu , Daniel Almeida , Gert Wollny , Gustavo Padovan , Tomeu Vizoso , Maarten Lankhorst , Maxime Ripard , Rob Herring , Steven Price , Alyssa Rosenzweig , Rob Clark , Emil Velikov , Robin Murphy , Dmitry Osipenko , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, virtualization@lists.linux-foundation.org References: <01506516-ab2f-cb6e-7507-f2a3295efb59@collabora.com> <83e68918-68de-c0c6-6f9b-e94d34b19383@collabora.com> <4d08b382-0076-1ea2-b565-893d50b453cb@collabora.com> <56787b70-fb64-64da-6006-d3aa3ed59d12@gmail.com> <3a362c32-870c-1d73-bba6-bbdcd62dc326@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 5.10.0-8-amd64 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 11, 2022 at 06:40:32PM +0300, Dmitry Osipenko wrote: > On 5/11/22 18:29, Daniel Vetter wrote: > > On Wed, May 11, 2022 at 06:14:00PM +0300, Dmitry Osipenko wrote: > >> On 5/11/22 17:24, Christian König wrote: > >>> Am 11.05.22 um 15:00 schrieb Daniel Vetter: > >>>> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote: > >>>>> [SNIP] > >>>>> Since vmapping implies implicit pinning, we can't use a separate lock in > >>>>> drm_gem_shmem_vmap() because we need to protect the > >>>>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to > >>>>> pin the pages and requires the dma_resv_lock to be locked. > >>>>> > >>>>> Hence the problem is: > >>>>> > >>>>> 1. If dma-buf importer holds the dma_resv_lock and invokes > >>>>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall > >>>>> not take the dma_resv_lock. > >>>>> > >>>>> 2. Since dma-buf locking convention isn't specified, we can't assume > >>>>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap(). > >>>>> > >>>>> The possible solutions are: > >>>>> > >>>>> 1. Specify the dma_resv_lock convention for dma-bufs and make all > >>>>> drivers to follow it. > >>>>> > >>>>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap(). > >>>>> Other non-DRM drivers will get the lockdep warning. > >>>>> > >>>>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock > >>>>> if dma-buf importer holds the lock. > >>>>> > >>>>> ... > >>>> Yeah this is all very annoying. > >>> Ah, yes that topic again :) > >>> > >>> I think we could relatively easily fix that by just defining and > >>> enforcing that the dma_resv_lock must have be taken by the caller when > >>> dma_buf_vmap() is called. > >>> > >>> A two step approach should work: > >>> 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and > >>> remove all lock taking from the vmap callback implementations. > >>> 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap() > >>> and enforce that the function is called with the lock held. > >> I've doubts about the need to move out the dma_resv_lock() into the > >> callers of dma_buf_vmap().. > >> > >> I looked through all the dma_buf_vmap() users and neither of them > >> interacts with dma_resv_lock() at all, i.e. nobody takes the lock > >> in/outside of dma_buf_vmap(). Hence it's easy and more practical to make > >> dma_buf_mmap/vmap() to take the dma_resv_lock by themselves. > > i915_gem_dmabuf_vmap -> i915_gem_object_pin_map_unlocked -> > > i915_gem_object_lock -> dma_resv_lock > > > > And all the ttm drivers should work similarly. So there's definitely > > drivers which grab dma_resv_lock from their vmap callback. > > Grr.. I'll take another look. > > >> It's unclear to me which driver may ever want to do the mapping under > >> the dma_resv_lock. But if we will ever have such a driver that will need > >> to map imported buffer under dma_resv_lock, then we could always add the > >> dma_buf_vmap_locked() variant of the function. In this case the locking > >> rule will sound like this: > >> > >> "All dma-buf importers are responsible for holding the dma-reservation > >> lock around the dmabuf->ops->mmap/vmap() calls." > > Are you okay with this rule? Yeah I think long-term it's where we want to be, just trying to find clever ways to get there. And I think Christian agrees with that? > >>> It shouldn't be that hard to clean up. The last time I looked into it my > >>> main problem was that we didn't had any easy unit test for it. > >> Do we have any tests for dma-bufs at all? It's unclear to me what you > >> are going to test in regards to the reservation locks, could you please > >> clarify? > > Unfortunately not really :-/ Only way really is to grab a driver which > > needs vmap (those are mostly display drivers) on an imported buffer, and > > see what happens. > > > > 2nd best is liberally sprinkling lockdep annotations all over the place > > and throwing it at intel ci (not sure amd ci is accessible to the public) > > and then hoping that's good enough. Stuff like might_lock and > > dma_resv_assert_held. > > Alright So throwing it at intel-gfx-ci can't hurt I think, but that only covers i915 so doesn't really help with the bigger issue of catching all the drivers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch