Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Mar 2023 14:56:28 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Zhao Liu <zhao1.liu@linux.intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Matthew Auld <matthew.auld@intel.com>,
        Thomas =?iso-8859-1?Q?Hellstr=F6m?= 
        <thomas.hellstrom@linux.intel.com>,
        Nirmoy Das <nirmoy.das@intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
CC:     Ira Weiny <ira.weiny@intel.com>,
        "Fabio M . De Francesco" <fmdefrancesco@gmail.com>,
        Zhenyu Wang <zhenyu.z.wang@intel.com>,
        Zhao Liu <zhao1.liu@intel.com>,
        Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCH v2 1/9] drm/i915: Use kmap_local_page() in
 gem/i915_gem_object.c
Message-ID: <6426058c566b7_375f7e294d9@iweiny-mobl.notmuch>
References: <20230329073220.3982460-1-zhao1.liu@linux.intel.com>
 <20230329073220.3982460-2-zhao1.liu@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230329073220.3982460-2-zhao1.liu@linux.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?SyQkSn62Twd1f3EjB32whDdDBW7kBQ/UImKaMVRmWB4tedw3O/xG1CDDbeEV?=
 =?us-ascii?Q?GHXg/b7IQbmmFsAqz35S+fFahxc5wN2ySufAPSWjOGPESOr80MQ7DQN7RCxn?=
 =?us-ascii?Q?aZO02PMDOmDc56UydEsvo9UOWgYbGLhibWb/RppXkjNOTglFsUssnkqM2zfh?=
 =?us-ascii?Q?9nwe8KUSIVtKkGiRaNJk3kDzD1N23l3pmFeMFcOHd/GBrr9KKMAsY9HB3t+/?=
 =?us-ascii?Q?3oVYfwOA1XzTxYpiEet+9McciJPDnAlk/H8VD59KTs6Izx/RpdkVToEpLUfw?=
 =?us-ascii?Q?ANosL7TGLVuqXHI0n47R0JyyEEQHcZdo7EmrEW4VlVH15hCWCdm6dIOJD6c9?=
 =?us-ascii?Q?CI3Dp+tIMLxYp2ZkAv9gnghdUzSt2q+pu9r/sSV0BKEO9lIk2uSvayXUJp/q?=
 =?us-ascii?Q?zKi3vvK3VH60749VtzmI46NwLWOhh65FL32z4tHo9COfI3dZm3F9qYbJpjmP?=
 =?us-ascii?Q?gjUpSSTI0tpyr+y00+A4mbuyXo3xW+QYezUBt8wnOo6gYregFME68pCfKREz?=
 =?us-ascii?Q?07IXmjGIcveaD5S1P6cd34I97PLUkNlY9D4FZgLQNCmvrMZiCuMmJxli+TZd?=
 =?us-ascii?Q?p/ib+dm6ppY6+BJXdKAsmelBRhG/KLozM8d8pk/X69moHlaZnendXnsFKY/s?=
 =?us-ascii?Q?yONa5gudDn3E0as4lfByCpuoX16fTr78cjOhF6v3XWqZJOtVETIQMw42XyzT?=
 =?us-ascii?Q?4leGDYgkeACO74DNUypj3DgEWshxbW44V4Xdplsev2d8sqWQ7HkcsgUOyjw+?=
 =?us-ascii?Q?xhOhC5KOXiXmKQTyi4cLSWYelxAY98Lg+xJC31KSSGT4i59UoqlkoCVcCEjr?=
 =?us-ascii?Q?dUvBWBX+zw+yWVadFoFdYh5+ArWssrskBQKH4rKAzTNjGOn75ECK7UQVq9zZ?=
 =?us-ascii?Q?nEuaiLhzGbHv3gHBED0Thmf6u0gHalaMJpG23yBmzS9ugVWoCbUzvAK6xnyv?=
 =?us-ascii?Q?cmiGvKfQAa4dW7d5SZvh5ezXUzlElLq/5rjByU6UlEdRaJTw05sKu2G/L5zi?=
 =?us-ascii?Q?reEx7cMkfudwIWJUx7+kLMUymXewvLI+gYbetcB7ZQv0ZK73PHLUq5Az82wQ?=
 =?us-ascii?Q?R7AWJmQlFNjIT6uEWkFrH7HQecyLCECecK0tBc3RE038QN/83l6mTU9D9XgF?=
 =?us-ascii?Q?4c82Sg+PBTU8kQ75T088wRa7XK9bDa5q9fKiMy+f0sLB3mKFwV+VrNnIq+p3?=
 =?us-ascii?Q?Y30YYqVj5FbeIV3yygy/2dDn+HhlS3OGqmebl0zz0XfT7LoAqcgrL51do6bt?=
 =?us-ascii?Q?xa8rfBaUyR4nkCywii13QmTDjPEho+bP21MfE0vSJGlB70iTdYBdFPr2pZA9?=
 =?us-ascii?Q?2R4/IlgwmAyRLUJTqM98CVV3rfxECO6Pdc28Mk/BeUfffYbjxNfU+sMIZbVq?=
 =?us-ascii?Q?qVal3KFwLABmmbmxzTKoDSwTr28eBRkznXRzl9lnn3nrABTGNU5kY3Sg+d0F?=
 =?us-ascii?Q?79m9Qi4vyPHuIvE3cPWyNvpF/mSztPDUW1c3GkfzgnM0DToOqcAM84MZUDtD?=
 =?us-ascii?Q?tVBorGdKTqxC9mYVAeh5yo03Rfx3J0ffnSEY4E8ybnwWCLnOm19xJmXGOywW?=
 =?us-ascii?Q?vhHczSG5CsQznkXlkaj7CznB+uPOoHoIXrXPSuFf?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 47f6de1b-aac3-4251-0a57-08db3169a047
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 21:56:33.5825
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PoHgXG6P4lzISz4iEejcNjhu8n1WUaWHvLfSlc5Jp1MI0p6BsFFgbdN8L2tnkUCZWT6PPw319UDSWZEN+ACTaw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7547
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE,
        SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1], and this patch converts the call from
> kmap_atomic() to kmap_local_page().
> 
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption (the preemption is
> disabled for !PREEMPT_RT case, otherwise it only disables migration).
> 
> With kmap_local_page(), we can avoid the often unwanted side effect of
> unnecessary page faults and preemption disables.
> 
> There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> need to disable pagefaults and preemption for mapping:
> 
> 1. The flush operation is safe. In drm/i915/gem/i915_gem_object.c,
> i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range() to
> use CLFLUSHOPT or WBINVD to flush. Since CLFLUSHOPT is global on x86
> and WBINVD is called on each cpu in drm_clflush_virt_range(), the flush
> operation is global.
> 
> 2. Any context switch caused by preemption or page faults (page fault
> may cause sleep) doesn't affect the validity of local mapping.
> 
> Therefore, i915_gem_object_read_from_page_kmap() is a function where
> the use of kmap_local_page() in place of kmap_atomic() is correctly
> suited.
> 
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
> 
> And remove the redundant variable that stores the address of the mapped
> page since kunmap_local() can accept any pointer within the page.
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> 
> v2:
> * Dropped hot plug related description since it has nothing to do with
>   kmap_local_page().
> * Rebased on f47e630 (drm/i915/gem: Typecheck page lookups) to keep
>   the "idx" variable of type pgoff_t here.
> * Added description of the motivation of using kmap_local_page().
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>