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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 3CD15C4167B for ; Thu, 7 Dec 2023 13:07:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id EE78F10E8AA; Thu, 7 Dec 2023 13:07:01 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.9]) by gabe.freedesktop.org (Postfix) with ESMTPS id C1BCC10E8AA for ; Thu, 7 Dec 2023 13:07:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701954421; x=1733490421; h=message-id:date:mime-version:subject:from:to:cc: references:in-reply-to:content-transfer-encoding; bh=qnWZbIPdnUbooEApKSeYmCZ5tmw7aJzc4bQxDEkcYJE=; b=dQ/cR8oD7sOwaayRZ7jM1PhPmOHB1+jPe/MhvIXDNjf594leL/Y2FqFx T3gY4bXyT29WsSlNlalTfFtOr95SLgxdE5E3L5/sMgqPdc3vdh5Roup1Q Mg8JERJGxNJtv6CRuyTTHbCpFc2ZvxVaKIqAxU1UHP8VpfMYkj4d2c3zT 7G6h8WGwNqNtocffuNBq9SP0A0MTDGHRsFA7frfuqHHEC+LLKK/Xnm3vV iUvkieR6p75+3YeMklCwH6y6m3iHW4amlkNHzR7GJ9jZwYWuASoHMoK4U izkpFriroQ5wXL6L59969dMVE/VzcZ9Bu9i0/Yc4KCEtSm7LY3yscoje+ g==; X-IronPort-AV: E=McAfee;i="6600,9927,10916"; a="1107245" X-IronPort-AV: E=Sophos;i="6.04,256,1695711600"; d="scan'208";a="1107245" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmvoesa103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2023 05:07:00 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10916"; a="915578766" X-IronPort-AV: E=Sophos;i="6.04,256,1695711600"; d="scan'208";a="915578766" Received: from nbathi-mobl.ger.corp.intel.com (HELO [10.252.28.51]) ([10.252.28.51]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2023 05:06:58 -0800 Message-ID: <381b6034-cdbf-4259-ab28-e7858fd750dd@intel.com> Date: Thu, 7 Dec 2023 13:06:57 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] drm/xe/dgfx: Block rpm for active mmap mappings Content-Language: en-GB From: Matthew Auld To: Badal Nilawar , intel-xe@lists.freedesktop.org References: <20231206133421.3295163-1-badal.nilawar@intel.com> <20231206133421.3295163-2-badal.nilawar@intel.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: rodrigo.vivi@intel.com Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On 07/12/2023 11:26, Matthew Auld wrote: > On 06/12/2023 13:34, Badal Nilawar wrote: >> Block rpm for discrete cards when mmap mappings are active. >> Ideally rpm wake ref should be taken in vm_open call and put in vm_close >> call but it is seen that vm_open doesn't get called for xe_gem_vm_ops. >> Therefore rpm wake ref is being get in xe_drm_gem_ttm_mmap and put >> in vm_close. >> >> Cc: Rodrigo Vivi >> Cc: Anshuman Gupta >> Signed-off-by: Badal Nilawar >> --- >>   drivers/gpu/drm/xe/xe_bo.c | 35 +++++++++++++++++++++++++++++++++-- >>   1 file changed, 33 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c >> index 72dc4a4eed4e..5741948a2a51 100644 >> --- a/drivers/gpu/drm/xe/xe_bo.c >> +++ b/drivers/gpu/drm/xe/xe_bo.c >> @@ -15,6 +15,7 @@ >>   #include >>   #include >> +#include "i915_drv.h" > > Do we need this? > >>   #include "xe_device.h" >>   #include "xe_dma_buf.h" >>   #include "xe_drm_client.h" >> @@ -1158,17 +1159,47 @@ static vm_fault_t xe_gem_fault(struct vm_fault >> *vmf) >>       return ret; >>   } >> +static void xe_ttm_bo_vm_close(struct vm_area_struct *vma) >> +{ >> +    struct ttm_buffer_object *tbo = vma->vm_private_data; >> +    struct drm_device *ddev = tbo->base.dev; >> +    struct xe_device *xe = to_xe_device(ddev); >> + >> +    ttm_bo_vm_close(vma); >> + >> +    if (tbo->resource->bus.is_iomem) >> +        xe_device_mem_access_put(xe); Are you sure this works as expected? Say if the user partially unmaps something? map = mmap(obj, size); unmap(map, size/2); unmap(map, size); That would be one mmap but multiple vm_close calls leading to an imbalance in the RPM ref. I think we need the access_get in the vm_open also? >> +} >> + >>   static const struct vm_operations_struct xe_gem_vm_ops = { >>       .fault = xe_gem_fault, >>       .open = ttm_bo_vm_open, >> -    .close = ttm_bo_vm_close, >> +    .close = xe_ttm_bo_vm_close, >>       .access = ttm_bo_vm_access >>   }; >> +int xe_drm_gem_ttm_mmap(struct drm_gem_object *gem, >> +            struct vm_area_struct *vma) >> +{ >> +    struct ttm_buffer_object *tbo = drm_gem_ttm_of_gem(gem); >> +    struct drm_device *ddev = tbo->base.dev; >> +    struct xe_device *xe = to_xe_device(ddev); >> +    int ret; >> + >> +    ret = drm_gem_ttm_mmap(gem, vma); >> +    if (ret < 0) >> +        return ret; >> + >> +    if (tbo->resource->bus.is_iomem) >> +        xe_device_mem_access_get(xe); > > Checking is_iomem outside of the usual locking is racy. One issue here > is that is_iomem can freely change at any point (like at fault time) so > when vm_close is called you can easily get an an unbalanced RPM ref > count. For example io_mem is false here but later becomes true in > bo_vm_close and then we call mem_access_put even though we never called > mem_access_get. > > Maybe check the possible placements of the object instead since that is > immutable? > >> + >> +    return 0; >> +} >> + >>   static const struct drm_gem_object_funcs xe_gem_object_funcs = { >>       .free = xe_gem_object_free, >>       .close = xe_gem_object_close, >> -    .mmap = drm_gem_ttm_mmap, >> +    .mmap = xe_drm_gem_ttm_mmap, >>       .export = xe_gem_prime_export, >>       .vm_ops = &xe_gem_vm_ops, >>   };