Re: [PATCH v1 1/1] drm/xe: fix the ERR_PTR() returned on failure to allocate tiny pt

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Nov 21, 2024 at 10:20:58PM +0100, Mirsad Todorovac wrote:
> Running coccinelle spatch gave the following warning:
> 
> ./drivers/gpu/drm/xe/tests/xe_migrate.c:226:5-11: inconsistent IS_ERR and PTR_ERR on line 228.
> 
> The code reports PTR_ERR(pt) when IS_ERR(tiny) is checked:
> 
> → 211         pt = xe_bo_create_pin_map(xe, tile, m->q->vm, XE_PAGE_SIZE,
>   212                                   ttm_bo_type_kernel,
>   213                                   XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>   214                                   XE_BO_FLAG_PINNED);
>   215         if (IS_ERR(pt)) {
>   216                 KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
>   217                            PTR_ERR(pt));
>   218                 goto free_big;
>   219         }
>   220
>   221         tiny = xe_bo_create_pin_map(xe, tile, m->q->vm,
> → 222                                     2 * SZ_4K,
>   223                                     ttm_bo_type_kernel,
>   224                                     XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>   225                                     XE_BO_FLAG_PINNED);
> → 226         if (IS_ERR(tiny)) {
> → 227                 KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
> → 228                            PTR_ERR(pt));
>   229                 goto free_pt;
>   230         }
> 
> Now, the IS_ERR(tiny) and the corresponding PTR_ERR(pt) do not match.
> 
> Returning PTR_ERR(tiny), as the last failed function call, seems logical.
> 
> Fixes: dd08ebf6c3525 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Francois Dugast <francois.dugast@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Philippe Lecluse <philippe.lecluse@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Faith Ekstrand <faith.ekstrand@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Akshata Jahagirdar <akshata.jahagirdar@intel.com>
> Cc: David Kershner <david.kershner@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

I clean up this cc list. no reason for this simple patch to have this huge list.

I also fixed the checkpatch complains in the commit message.

And pushed to drm-xe-next

Thanks for the patch.

> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> ---
>  v1:
> 	There is also the logical problem which this patch developer is not qualified to fix:
> 	first the fake pt tries to allocate size of (1 << (XE_PT_SHIFT=12)) = 4096 bytes,
> 	then "tiny pt" tries to allocate 2 * SZ_4K, twice as much. Is this what was meant
> 	to accomplish?
> 
> 	drivers/gpu/drm/xe/xe_bo.h#L46:
> 	#define XE_PTE_SHIFT			12
> 	#define XE_PAGE_SIZE			(1 << XE_PTE_SHIFT)
> 
> 	include/linux/sizes.h#L23:
> 	#define SZ_4K				0x00001000
> 
>  drivers/gpu/drm/xe/tests/xe_migrate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
> index 1a192a2a941b..3bbdb362d6f0 100644
> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
> @@ -224,8 +224,8 @@ static void xe_migrate_sanity_test(struct xe_migrate *m, struct kunit *test)
>  				    XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>  				    XE_BO_FLAG_PINNED);
>  	if (IS_ERR(tiny)) {
> -		KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
> -			   PTR_ERR(pt));
> +		KUNIT_FAIL(test, "Failed to allocate tiny fake pt: %li\n",
> +			   PTR_ERR(tiny));
>  		goto free_pt;
>  	}
>  
> -- 
> 2.43.0
>