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 6E456C433FE for ; Tue, 8 Nov 2022 09:48:49 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 27B4C10E0E8; Tue, 8 Nov 2022 09:48:48 +0000 (UTC) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id C8DBF10E0E8 for ; Tue, 8 Nov 2022 09:48:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667900924; x=1699436924; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=6itfsQ/I/LMqAn8yKkNcfEKrXRtHcm/Cw3+m0IqlURU=; b=TsXf19rGy7cWXGNUVgNKVASwFECloEhcYAgMHpZ3bnPApPYfMXIzyIDu pKOSaQupYOHRcSdKLIn9fY4QsH+3eq72GSsqeCdZhpfRzFfMyMNBTHTDY 8++hNr5bky5LbLBomJLHkZDRiCARYYMvv+soTzRfw7/9p7lClTp/cAc+u M+69lTtX7aeIogAOTurvxYL6xjwJB+GT6igiOJTmQB6ZsBZTxZiot/He0 ywCxeVdJbeEgkmu1/Nj8DrE8QDBpzGovi++kCQCq5kVEreLcxc7s/oMEs PuXhsiafv5jj84OjRsoa37QnL5nrracjtuXR7TlpFEeoscyG8dgNw9bUD g==; X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="311810197" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="311810197" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 01:48:44 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="811182153" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="811182153" Received: from shylandx-mobl2.ger.corp.intel.com (HELO [10.213.210.50]) ([10.213.210.50]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 01:48:42 -0800 Message-ID: Date: Tue, 8 Nov 2022 09:48:39 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Content-Language: en-US To: Matthew Auld , intel-gfx@lists.freedesktop.org References: <20221107173435.68116-1-matthew.auld@intel.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc In-Reply-To: <20221107173435.68116-1-matthew.auld@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Intel-gfx] [PATCH] drm/i915: use i915_sg_dma_sizes() for all backends X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrzej Hajda Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 07/11/2022 17:34, Matthew Auld wrote: > We rely on page_sizes.sg in setup_scratch_page() reporting the correct > value if the underlying sgl is not contiguous, however in > get_pages_internal() we are only looking at the layout of the created > pages when calculating the sg_page_sizes, and not the final sgl, which > could in theory be completely different. In such a situation we might > incorrectly think we have a 64K scratch page, when it is actually only > 4K or similar split over multiple non-contiguous entries, which could > lead to broken behaviour when touching the scratch space within the > padding of a 64K GTT page-table. For most of the other backends we > already just call i915_sg_dma_sizes() on the final mapping, so rather > just move that into __i915_gem_object_set_pages() to avoid such issues > coming back to bite us later. > > Suggested-by: Tvrtko Ursulin > Signed-off-by: Matthew Auld > Cc: Stuart Summers > Cc: Andrzej Hajda > --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 4 +--- > drivers/gpu/drm/i915/gem/i915_gem_internal.c | 5 +---- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 7 +++---- > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 9 +++------ > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +-- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +--- > drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 2 +- > drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 10 +++------- > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 5 +---- > drivers/gpu/drm/i915/selftests/mock_region.c | 2 +- > 13 files changed, 19 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index ec6f7ae47783..1df74f7aa3dc 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -238,7 +238,6 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) > { > struct drm_i915_private *i915 = to_i915(obj->base.dev); > struct sg_table *sgt; > - unsigned int sg_page_sizes; > > assert_object_held(obj); > > @@ -262,8 +261,7 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) > (!HAS_LLC(i915) && !IS_DG1(i915))) > wbinvd_on_all_cpus(); > > - sg_page_sizes = i915_sg_dma_sizes(sgt->sgl); > - __i915_gem_object_set_pages(obj, sgt, sg_page_sizes); > + __i915_gem_object_set_pages(obj, sgt); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > index 629acb403a2c..f66bcefc09ec 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > @@ -35,7 +35,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > struct drm_i915_private *i915 = to_i915(obj->base.dev); > struct sg_table *st; > struct scatterlist *sg; > - unsigned int sg_page_sizes; > unsigned int npages; > int max_order = MAX_ORDER; > unsigned int max_segment; > @@ -64,7 +63,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > > sg = st->sgl; > st->nents = 0; > - sg_page_sizes = 0; > > do { > int order = min(fls(npages) - 1, max_order); > @@ -83,7 +81,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > } while (1); > > sg_set_page(sg, page, PAGE_SIZE << order, 0); > - sg_page_sizes |= PAGE_SIZE << order; > st->nents++; > > npages -= 1 << order; > @@ -105,7 +102,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > goto err; > } > > - __i915_gem_object_set_pages(obj, st, sg_page_sizes); > + __i915_gem_object_set_pages(obj, st); > > return 0; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 6b9ecff42bb5..3db53769864c 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -403,8 +403,7 @@ i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, > unsigned long n); > > void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, > - struct sg_table *pages, > - unsigned int sg_page_sizes); > + struct sg_table *pages); > > int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > index 16f845663ff2..05a27723ebb8 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > @@ -16,8 +16,7 @@ > #include "i915_gem_mman.h" > > void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, > - struct sg_table *pages, > - unsigned int sg_page_sizes) > + struct sg_table *pages) > { > struct drm_i915_private *i915 = to_i915(obj->base.dev); > unsigned long supported = RUNTIME_INFO(i915)->page_sizes; > @@ -45,8 +44,8 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, > > obj->mm.pages = pages; > > - GEM_BUG_ON(!sg_page_sizes); > - obj->mm.page_sizes.phys = sg_page_sizes; > + obj->mm.page_sizes.phys = i915_sg_dma_sizes(pages->sgl); > + GEM_BUG_ON(!obj->mm.page_sizes.phys); > > /* > * Calculate the supported page-sizes which fit into the given > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > index 0d0e46dae559..68453572275b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > @@ -79,7 +79,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj) > > /* We're no longer struct page backed */ > obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE; > - __i915_gem_object_set_pages(obj, st, sg->length); > + __i915_gem_object_set_pages(obj, st); > > return 0; > > @@ -209,11 +209,8 @@ static int i915_gem_object_shmem_to_phys(struct drm_i915_gem_object *obj) > return 0; > > err_xfer: > - if (!IS_ERR_OR_NULL(pages)) { > - unsigned int sg_page_sizes = i915_sg_dma_sizes(pages->sgl); > - > - __i915_gem_object_set_pages(obj, pages, sg_page_sizes); > - } > + if (!IS_ERR_OR_NULL(pages)) > + __i915_gem_object_set_pages(obj, pages); > return err; > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index 2f7804492cd5..9c759df700ca 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -247,7 +247,7 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > if (i915_gem_object_can_bypass_llc(obj)) > obj->cache_dirty = true; > > - __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl)); > + __i915_gem_object_set_pages(obj, st); > > return 0; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > index 0c70711818ed..bc9521078807 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > @@ -628,7 +628,7 @@ static int i915_gem_object_get_pages_stolen(struct drm_i915_gem_object *obj) > sg_dma_len(pages->sgl), > POISON_INUSE); > > - __i915_gem_object_set_pages(obj, pages, obj->stolen->size); > + __i915_gem_object_set_pages(obj, pages); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 25129af70f70..fb9ba712e63c 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -815,8 +815,7 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj, > > GEM_BUG_ON(obj->mm.rsgt); > obj->mm.rsgt = rsgt; > - __i915_gem_object_set_pages(obj, &rsgt->table, > - i915_sg_dma_sizes(rsgt->table.sgl)); > + __i915_gem_object_set_pages(obj, &rsgt->table); > } > > GEM_BUG_ON(bo->ttm && ((obj->base.size >> PAGE_SHIFT) < bo->ttm->num_pages)); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index ca7a388ba2bf..9348b1804d53 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -131,7 +131,6 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > const unsigned long num_pages = obj->base.size >> PAGE_SHIFT; > unsigned int max_segment = i915_sg_segment_size(obj->base.dev->dev); > struct sg_table *st; > - unsigned int sg_page_sizes; > struct page **pvec; > int ret; > > @@ -170,8 +169,7 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > if (i915_gem_object_can_bypass_llc(obj)) > obj->cache_dirty = true; > > - sg_page_sizes = i915_sg_dma_sizes(st->sgl); > - __i915_gem_object_set_pages(obj, st, sg_page_sizes); > + __i915_gem_object_set_pages(obj, st); > > return 0; > > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c > index f963b8e1e37b..cbd9b624a788 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c > @@ -68,7 +68,7 @@ static int huge_get_pages(struct drm_i915_gem_object *obj) > if (i915_gem_gtt_prepare_pages(obj, pages)) > goto err; > > - __i915_gem_object_set_pages(obj, pages, PAGE_SIZE); > + __i915_gem_object_set_pages(obj, pages); > > return 0; > > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > index 0cb99e75b0bc..beaf27e09e8a 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > @@ -136,7 +136,7 @@ static int get_huge_pages(struct drm_i915_gem_object *obj) > goto err; > > GEM_BUG_ON(sg_page_sizes != obj->mm.page_mask); > - __i915_gem_object_set_pages(obj, st, sg_page_sizes); > + __i915_gem_object_set_pages(obj, st); > > return 0; > > @@ -210,7 +210,6 @@ static int fake_get_huge_pages(struct drm_i915_gem_object *obj) > const u64 max_len = rounddown_pow_of_two(UINT_MAX); > struct sg_table *st; > struct scatterlist *sg; > - unsigned int sg_page_sizes; > u64 rem; > > st = kmalloc(sizeof(*st), GFP); > @@ -226,7 +225,6 @@ static int fake_get_huge_pages(struct drm_i915_gem_object *obj) > rem = obj->base.size; > sg = st->sgl; > st->nents = 0; > - sg_page_sizes = 0; > do { > unsigned int page_size = get_largest_page_size(i915, rem); > unsigned int len = min(page_size * div_u64(rem, page_size), > @@ -239,8 +237,6 @@ static int fake_get_huge_pages(struct drm_i915_gem_object *obj) > sg_dma_len(sg) = len; > sg_dma_address(sg) = page_size; > > - sg_page_sizes |= len; > - > st->nents++; > > rem -= len; > @@ -254,7 +250,7 @@ static int fake_get_huge_pages(struct drm_i915_gem_object *obj) > > i915_sg_trim(st); > > - __i915_gem_object_set_pages(obj, st, sg_page_sizes); > + __i915_gem_object_set_pages(obj, st); > > return 0; > } > @@ -286,7 +282,7 @@ static int fake_get_huge_pages_single(struct drm_i915_gem_object *obj) > sg_dma_len(sg) = obj->base.size; > sg_dma_address(sg) = page_size; > > - __i915_gem_object_set_pages(obj, st, sg->length); > + __i915_gem_object_set_pages(obj, st); > > return 0; > #undef GFP > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > index 27c733b00976..eae7d947d7de 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > @@ -61,7 +61,6 @@ static int fake_get_pages(struct drm_i915_gem_object *obj) > #define PFN_BIAS 0x1000 > struct sg_table *pages; > struct scatterlist *sg; > - unsigned int sg_page_sizes; > typeof(obj->base.size) rem; > > pages = kmalloc(sizeof(*pages), GFP); > @@ -74,7 +73,6 @@ static int fake_get_pages(struct drm_i915_gem_object *obj) > return -ENOMEM; > } > > - sg_page_sizes = 0; > rem = obj->base.size; > for (sg = pages->sgl; sg; sg = sg_next(sg)) { > unsigned long len = min_t(typeof(rem), rem, BIT(31)); > @@ -83,13 +81,12 @@ static int fake_get_pages(struct drm_i915_gem_object *obj) > sg_set_page(sg, pfn_to_page(PFN_BIAS), len, 0); > sg_dma_address(sg) = page_to_phys(sg_page(sg)); > sg_dma_len(sg) = len; > - sg_page_sizes |= len; > > rem -= len; > } > GEM_BUG_ON(rem); > > - __i915_gem_object_set_pages(obj, pages, sg_page_sizes); > + __i915_gem_object_set_pages(obj, pages); > > return 0; > #undef GFP > diff --git a/drivers/gpu/drm/i915/selftests/mock_region.c b/drivers/gpu/drm/i915/selftests/mock_region.c > index bac21fe84ca5..6324eb32d4dd 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_region.c > +++ b/drivers/gpu/drm/i915/selftests/mock_region.c > @@ -41,7 +41,7 @@ static int mock_region_get_pages(struct drm_i915_gem_object *obj) > } > > pages = &obj->mm.rsgt->table; > - __i915_gem_object_set_pages(obj, pages, i915_sg_dma_sizes(pages->sgl)); > + __i915_gem_object_set_pages(obj, pages); > > return 0; > Reviewed-by: Tvrtko Ursulin Regards, Tvrtko