All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 14:37 ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-07 14:37 UTC (permalink / raw)
  To: amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, Arunpravin, christian.koenig, matthew.auld

place BUG_ON(order < min_order) outside do..while
loop as it fails Unigine Heaven benchmark.

Unigine Heaven has buffer allocation requests for
example required pages are 161 and alignment request
is 128. To allocate the remaining 33 pages, continues
the iteration to find the order value which is 5 and
when it compares with min_order = 7, enables the
BUG_ON(). To avoid this problem, placed the BUG_ON
check outside of do..while loop.

Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/drm_buddy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..ed94c56b720f 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
 	order = fls(pages) - 1;
 	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
 
+	BUG_ON(order < min_order);
+
 	do {
 		order = min(order, (unsigned int)fls(pages) - 1);
 		BUG_ON(order > mm->max_order);
-		BUG_ON(order < min_order);
 
 		do {
 			if (flags & DRM_BUDDY_RANGE_ALLOCATION)

base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 14:37 ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-07 14:37 UTC (permalink / raw)
  To: amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, Arunpravin, christian.koenig, matthew.auld

place BUG_ON(order < min_order) outside do..while
loop as it fails Unigine Heaven benchmark.

Unigine Heaven has buffer allocation requests for
example required pages are 161 and alignment request
is 128. To allocate the remaining 33 pages, continues
the iteration to find the order value which is 5 and
when it compares with min_order = 7, enables the
BUG_ON(). To avoid this problem, placed the BUG_ON
check outside of do..while loop.

Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/drm_buddy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..ed94c56b720f 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
 	order = fls(pages) - 1;
 	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
 
+	BUG_ON(order < min_order);
+
 	do {
 		order = min(order, (unsigned int)fls(pages) - 1);
 		BUG_ON(order > mm->max_order);
-		BUG_ON(order < min_order);
 
 		do {
 			if (flags & DRM_BUDDY_RANGE_ALLOCATION)

base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 14:37 ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-07 14:37 UTC (permalink / raw)
  To: amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, Arunpravin, christian.koenig, daniel, matthew.auld

place BUG_ON(order < min_order) outside do..while
loop as it fails Unigine Heaven benchmark.

Unigine Heaven has buffer allocation requests for
example required pages are 161 and alignment request
is 128. To allocate the remaining 33 pages, continues
the iteration to find the order value which is 5 and
when it compares with min_order = 7, enables the
BUG_ON(). To avoid this problem, placed the BUG_ON
check outside of do..while loop.

Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/drm_buddy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..ed94c56b720f 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
 	order = fls(pages) - 1;
 	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
 
+	BUG_ON(order < min_order);
+
 	do {
 		order = min(order, (unsigned int)fls(pages) - 1);
 		BUG_ON(order > mm->max_order);
-		BUG_ON(order < min_order);
 
 		do {
 			if (flags & DRM_BUDDY_RANGE_ALLOCATION)

base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
  (?)
  (?)
@ 2022-03-07 14:45 ` Jani Nikula
  2022-03-08 14:32   ` Arunpravin
  -1 siblings, 1 reply; 32+ messages in thread
From: Jani Nikula @ 2022-03-07 14:45 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, matthew.auld, christian.koenig, Arunpravin

On Mon, 07 Mar 2022, Arunpravin <Arunpravin.PaneerSelvam@amd.com> wrote:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
>
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.

How about turning these BUG_ON()s to WARN_ON()s with an error return?
What's the point in oopsing?

BR,
Jani.


>
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>  drivers/gpu/drm/drm_buddy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>  	order = fls(pages) - 1;
>  	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>  
> +	BUG_ON(order < min_order);
> +
>  	do {
>  		order = min(order, (unsigned int)fls(pages) - 1);
>  		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>  
>  		do {
>  			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
                   ` (2 preceding siblings ...)
  (?)
@ 2022-03-07 15:04 ` Patchwork
  -1 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2022-03-07 15:04 UTC (permalink / raw)
  To: Arunpravin; +Cc: intel-gfx

== Series Details ==

Series: drm: remove min_order BUG_ON check
URL   : https://patchwork.freedesktop.org/series/101108/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
54fbfb3df283 drm: remove min_order BUG_ON check
-:27: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#27: FILE: drivers/gpu/drm/drm_buddy.c:672:
+	BUG_ON(order < min_order);

total: 0 errors, 1 warnings, 0 checks, 12 lines checked



^ permalink raw reply	[flat|nested] 32+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
                   ` (3 preceding siblings ...)
  (?)
@ 2022-03-07 15:36 ` Patchwork
  -1 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2022-03-07 15:36 UTC (permalink / raw)
  To: Arunpravin; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]

== Series Details ==

Series: drm: remove min_order BUG_ON check
URL   : https://patchwork.freedesktop.org/series/101108/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11332 -> Patchwork_22502
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/index.html

Participating hosts (44 -> 40)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (5): shard-tglu fi-bsw-cyan shard-rkl shard-dg1 fi-bdw-samus 

Known issues
------------

  Here are the changes found in Patchwork_22502 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-skl-6600u:       NOTRUN -> [FAIL][1] ([i915#4547])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-bdw-5557u:       [PASS][2] -> [INCOMPLETE][3] ([i915#146])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-pnv-d510:        NOTRUN -> [SKIP][4] ([fdo#109271]) +57 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/fi-pnv-d510/igt@gem_huc_copy@huc-copy.html

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [PASS][5] -> [INCOMPLETE][6] ([i915#3921])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - {fi-tgl-dsi}:       [DMESG-FAIL][7] ([i915#541]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/fi-tgl-dsi/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/fi-tgl-dsi/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-6:          [DMESG-FAIL][9] ([i915#4957]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/bat-dg1-6/igt@i915_selftest@live@hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/bat-dg1-6/igt@i915_selftest@live@hangcheck.html
    - bat-dg1-5:          [DMESG-FAIL][11] ([i915#4494] / [i915#4957]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/bat-dg1-5/igt@i915_selftest@live@hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/bat-dg1-5/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-2}:       [DMESG-FAIL][13] ([i915#5087]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/bat-rpls-2/igt@i915_selftest@live@requests.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/bat-rpls-2/igt@i915_selftest@live@requests.html

  * igt@kms_flip@basic-flip-vs-modeset@a-edp1:
    - {bat-adlp-6}:       [DMESG-WARN][15] ([i915#3576]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/bat-adlp-6/igt@kms_flip@basic-flip-vs-modeset@a-edp1.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/bat-adlp-6/igt@kms_flip@basic-flip-vs-modeset@a-edp1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#5087]: https://gitlab.freedesktop.org/drm/intel/issues/5087
  [i915#5193]: https://gitlab.freedesktop.org/drm/intel/issues/5193
  [i915#541]: https://gitlab.freedesktop.org/drm/intel/issues/541


Build changes
-------------

  * Linux: CI_DRM_11332 -> Patchwork_22502

  CI-20190529: 20190529
  CI_DRM_11332: 8025c79350b90e5a8029234d433578f12abbae2b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6366: 8c0bb07b7b4d9b724a897a7665fb9b1c450b2cb6 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22502: 54fbfb3df283ade8052a289018823dac3676bf44 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

54fbfb3df283 drm: remove min_order BUG_ON check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/index.html

[-- Attachment #2: Type: text/html, Size: 5793 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
  (?)
@ 2022-03-07 15:53   ` Christian König
  -1 siblings, 0 replies; 32+ messages in thread
From: Christian König @ 2022-03-07 15:53 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx; +Cc: alexander.deucher, matthew.auld

Am 07.03.22 um 15:37 schrieb Arunpravin:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
>
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.

Well using BUG_ON sounds like the wrong approach in the first place.

A BUG_ON() is only justified if you prevent further data corruption, 
e.g. when you detect for example a reference count overflow or similar.

In all other cases you should trigger a WARN_ON() and abort the 
operation with -EINVAL if possible.

Regards,
Christian.

>
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);
> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 15:53   ` Christian König
  0 siblings, 0 replies; 32+ messages in thread
From: Christian König @ 2022-03-07 15:53 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx; +Cc: alexander.deucher, matthew.auld

Am 07.03.22 um 15:37 schrieb Arunpravin:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
>
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.

Well using BUG_ON sounds like the wrong approach in the first place.

A BUG_ON() is only justified if you prevent further data corruption, 
e.g. when you detect for example a reference count overflow or similar.

In all other cases you should trigger a WARN_ON() and abort the 
operation with -EINVAL if possible.

Regards,
Christian.

>
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);
> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 15:53   ` Christian König
  0 siblings, 0 replies; 32+ messages in thread
From: Christian König @ 2022-03-07 15:53 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, matthew.auld, daniel

Am 07.03.22 um 15:37 schrieb Arunpravin:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
>
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.

Well using BUG_ON sounds like the wrong approach in the first place.

A BUG_ON() is only justified if you prevent further data corruption, 
e.g. when you detect for example a reference count overflow or similar.

In all other cases you should trigger a WARN_ON() and abort the 
operation with -EINVAL if possible.

Regards,
Christian.

>
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);
> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
  (?)
@ 2022-03-07 16:41   ` Matthew Auld
  -1 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-07 16:41 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 07/03/2022 14:37, Arunpravin wrote:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
> 
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.
> 
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);

Isn't the issue that we are allowing a size that is not aligned to the 
requested min_page_size? Should we not fix the caller(and throw a normal 
error here), or perhaps add the round_up() here instead?

i.e if someone does:

alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);

This will still trigger the BUG_ON() even if we move it out of the loop, 
AFAICT.

> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
> 
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 16:41   ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-07 16:41 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 07/03/2022 14:37, Arunpravin wrote:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
> 
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.
> 
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);

Isn't the issue that we are allowing a size that is not aligned to the 
requested min_page_size? Should we not fix the caller(and throw a normal 
error here), or perhaps add the round_up() here instead?

i.e if someone does:

alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);

This will still trigger the BUG_ON() even if we move it out of the loop, 
AFAICT.

> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
> 
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-07 16:41   ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-07 16:41 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel

On 07/03/2022 14:37, Arunpravin wrote:
> place BUG_ON(order < min_order) outside do..while
> loop as it fails Unigine Heaven benchmark.
> 
> Unigine Heaven has buffer allocation requests for
> example required pages are 161 and alignment request
> is 128. To allocate the remaining 33 pages, continues
> the iteration to find the order value which is 5 and
> when it compares with min_order = 7, enables the
> BUG_ON(). To avoid this problem, placed the BUG_ON
> check outside of do..while loop.
> 
> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 72f52f293249..ed94c56b720f 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   	order = fls(pages) - 1;
>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>   
> +	BUG_ON(order < min_order);

Isn't the issue that we are allowing a size that is not aligned to the 
requested min_page_size? Should we not fix the caller(and throw a normal 
error here), or perhaps add the round_up() here instead?

i.e if someone does:

alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);

This will still trigger the BUG_ON() even if we move it out of the loop, 
AFAICT.

> +
>   	do {
>   		order = min(order, (unsigned int)fls(pages) - 1);
>   		BUG_ON(order > mm->max_order);
> -		BUG_ON(order < min_order);
>   
>   		do {
>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
> 
> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm: remove min_order BUG_ON check
  2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
                   ` (6 preceding siblings ...)
  (?)
@ 2022-03-07 18:31 ` Patchwork
  -1 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2022-03-07 18:31 UTC (permalink / raw)
  To: Arunpravin; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 30258 bytes --]

== Series Details ==

Series: drm: remove min_order BUG_ON check
URL   : https://patchwork.freedesktop.org/series/101108/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11332_full -> Patchwork_22502_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (13 -> 13)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_22502_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_exec_schedule@deep@vcs0:
    - {shard-rkl}:        NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-5/igt@gem_exec_schedule@deep@vcs0.html

  * igt@i915_selftest@live@hangcheck:
    - {shard-rkl}:        [PASS][2] -> [INCOMPLETE][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-6/igt@i915_selftest@live@hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-5/igt@i915_selftest@live@hangcheck.html

  * {igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format}:
    - shard-iclb:         [PASS][4] -> [INCOMPLETE][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-iclb7/igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb2/igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format.html

  
Known issues
------------

  Here are the changes found in Patchwork_22502_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@feature_discovery@display-3x:
    - shard-glk:          NOTRUN -> [SKIP][6] ([fdo#109271]) +40 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@feature_discovery@display-3x.html

  * igt@gem_eio@in-flight-1us:
    - shard-tglb:         [PASS][7] -> [TIMEOUT][8] ([i915#3063])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglb6/igt@gem_eio@in-flight-1us.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb3/igt@gem_eio@in-flight-1us.html

  * igt@gem_exec_capture@pi@rcs0:
    - shard-iclb:         [PASS][9] -> [INCOMPLETE][10] ([i915#3371])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-iclb6/igt@gem_exec_capture@pi@rcs0.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb5/igt@gem_exec_capture@pi@rcs0.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-skl:          NOTRUN -> [FAIL][11] ([i915#2846])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-iclb:         [PASS][12] -> [FAIL][13] ([i915#2842])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-iclb1/igt@gem_exec_fair@basic-none-share@rcs0.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb3/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-kbl:          NOTRUN -> [FAIL][14] ([i915#2842])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-kbl:          [PASS][15] -> [FAIL][16] ([i915#2842])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-kbl1/igt@gem_exec_fair@basic-pace@vecs0.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl4/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@gem_huc_copy@huc-copy:
    - shard-tglb:         [PASS][17] -> [SKIP][18] ([i915#2190])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglb2/igt@gem_huc_copy@huc-copy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb7/igt@gem_huc_copy@huc-copy.html
    - shard-skl:          NOTRUN -> [SKIP][19] ([fdo#109271] / [i915#2190])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@heavy-multi:
    - shard-glk:          NOTRUN -> [SKIP][20] ([fdo#109271] / [i915#4613])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@gem_lmem_swapping@heavy-multi.html

  * igt@gem_lmem_swapping@heavy-verify-multi:
    - shard-kbl:          NOTRUN -> [SKIP][21] ([fdo#109271] / [i915#4613])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@gem_lmem_swapping@heavy-verify-multi.html

  * igt@gem_lmem_swapping@verify:
    - shard-apl:          NOTRUN -> [SKIP][22] ([fdo#109271] / [i915#4613]) +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@gem_lmem_swapping@verify.html

  * igt@gem_media_vme:
    - shard-skl:          NOTRUN -> [SKIP][23] ([fdo#109271]) +131 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@gem_media_vme.html

  * igt@gem_spin_batch@legacy-resubmit@vebox:
    - shard-glk:          [PASS][24] -> [DMESG-WARN][25] ([i915#118])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-glk2/igt@gem_spin_batch@legacy-resubmit@vebox.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk3/igt@gem_spin_batch@legacy-resubmit@vebox.html

  * igt@gem_userptr_blits@input-checking:
    - shard-apl:          NOTRUN -> [DMESG-WARN][26] ([i915#4991])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@gem_userptr_blits@input-checking.html
    - shard-iclb:         NOTRUN -> [DMESG-WARN][27] ([i915#4991])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb3/igt@gem_userptr_blits@input-checking.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         [PASS][28] -> [FAIL][29] ([i915#454])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-iclb7/igt@i915_pm_dc@dc6-psr.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb6/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-skl:          [PASS][30] -> [DMESG-FAIL][31] ([i915#541])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl9/igt@i915_selftest@live@gt_heartbeat.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl3/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_suspend@debugfs-reader:
    - shard-skl:          [PASS][32] -> [INCOMPLETE][33] ([i915#4939])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl10/igt@i915_suspend@debugfs-reader.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@i915_suspend@debugfs-reader.html

  * igt@kms_big_fb@linear-32bpp-rotate-180:
    - shard-glk:          NOTRUN -> [DMESG-WARN][34] ([i915#118])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_big_fb@linear-32bpp-rotate-180.html

  * igt@kms_big_fb@x-tiled-16bpp-rotate-90:
    - shard-iclb:         NOTRUN -> [SKIP][35] ([fdo#110725] / [fdo#111614])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb8/igt@kms_big_fb@x-tiled-16bpp-rotate-90.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip:
    - shard-apl:          NOTRUN -> [SKIP][36] ([fdo#109271] / [i915#3777])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip:
    - shard-skl:          NOTRUN -> [FAIL][37] ([i915#3743])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - shard-glk:          NOTRUN -> [SKIP][38] ([fdo#109271] / [i915#3777]) +1 similar issue
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - shard-skl:          NOTRUN -> [SKIP][39] ([fdo#109271] / [i915#3777]) +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_mc_ccs:
    - shard-skl:          NOTRUN -> [SKIP][40] ([fdo#109271] / [i915#3886]) +8 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl8/igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_mc_ccs.html

  * igt@kms_ccs@pipe-b-crc-primary-rotation-180-y_tiled_gen12_rc_ccs:
    - shard-iclb:         NOTRUN -> [SKIP][41] ([fdo#109278]) +5 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb8/igt@kms_ccs@pipe-b-crc-primary-rotation-180-y_tiled_gen12_rc_ccs.html

  * igt@kms_ccs@pipe-b-random-ccs-data-y_tiled_gen12_mc_ccs:
    - shard-kbl:          NOTRUN -> [SKIP][42] ([fdo#109271] / [i915#3886]) +1 similar issue
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@kms_ccs@pipe-b-random-ccs-data-y_tiled_gen12_mc_ccs.html

  * igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_rc_ccs_cc:
    - shard-glk:          NOTRUN -> [SKIP][43] ([fdo#109271] / [i915#3886]) +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc:
    - shard-apl:          NOTRUN -> [SKIP][44] ([fdo#109271] / [i915#3886]) +4 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc.html
    - shard-iclb:         NOTRUN -> [SKIP][45] ([fdo#109278] / [i915#3886])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb3/igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_chamelium@dp-hpd-for-each-pipe:
    - shard-iclb:         NOTRUN -> [SKIP][46] ([fdo#109284] / [fdo#111827]) +2 similar issues
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb8/igt@kms_chamelium@dp-hpd-for-each-pipe.html

  * igt@kms_color_chamelium@pipe-a-ctm-max:
    - shard-glk:          NOTRUN -> [SKIP][47] ([fdo#109271] / [fdo#111827]) +4 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_color_chamelium@pipe-a-ctm-max.html

  * igt@kms_color_chamelium@pipe-c-ctm-max:
    - shard-kbl:          NOTRUN -> [SKIP][48] ([fdo#109271] / [fdo#111827]) +2 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@kms_color_chamelium@pipe-c-ctm-max.html

  * igt@kms_color_chamelium@pipe-d-ctm-0-5:
    - shard-apl:          NOTRUN -> [SKIP][49] ([fdo#109271] / [fdo#111827]) +2 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl1/igt@kms_color_chamelium@pipe-d-ctm-0-5.html

  * igt@kms_color_chamelium@pipe-d-degamma:
    - shard-skl:          NOTRUN -> [SKIP][50] ([fdo#109271] / [fdo#111827]) +9 similar issues
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@kms_color_chamelium@pipe-d-degamma.html

  * igt@kms_content_protection@type1:
    - shard-iclb:         NOTRUN -> [SKIP][51] ([fdo#109300] / [fdo#111066]) +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb3/igt@kms_content_protection@type1.html

  * igt@kms_cursor_crc@pipe-a-cursor-512x512-offscreen:
    - shard-tglb:         NOTRUN -> [SKIP][52] ([fdo#109279] / [i915#3359])
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb3/igt@kms_cursor_crc@pipe-a-cursor-512x512-offscreen.html

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-iclb:         NOTRUN -> [SKIP][53] ([fdo#109274] / [fdo#109278])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb3/igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic.html

  * igt@kms_cursor_legacy@cursor-vs-flip-legacy:
    - shard-snb:          [PASS][54] -> [SKIP][55] ([fdo#109271])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-snb4/igt@kms_cursor_legacy@cursor-vs-flip-legacy.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-snb7/igt@kms_cursor_legacy@cursor-vs-flip-legacy.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          [PASS][56] -> [FAIL][57] ([i915#2346] / [i915#533])
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-apl:          [PASS][58] -> [INCOMPLETE][59] ([i915#180] / [i915#1982])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-apl2/igt@kms_fbcon_fbt@fbc-suspend.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl3/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_flip@2x-flip-vs-blocking-wf-vblank:
    - shard-tglb:         NOTRUN -> [SKIP][60] ([fdo#109274] / [fdo#111825]) +1 similar issue
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb3/igt@kms_flip@2x-flip-vs-blocking-wf-vblank.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1:
    - shard-kbl:          [PASS][61] -> [FAIL][62] ([i915#79])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-kbl7/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl4/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.html

  * igt@kms_flip@flip-vs-suspend@b-dp1:
    - shard-kbl:          [PASS][63] -> [INCOMPLETE][64] ([i915#636])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-kbl6/igt@kms_flip@flip-vs-suspend@b-dp1.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl3/igt@kms_flip@flip-vs-suspend@b-dp1.html

  * igt@kms_flip@flip-vs-suspend@c-dp1:
    - shard-apl:          [PASS][65] -> [DMESG-WARN][66] ([i915#180]) +1 similar issue
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-apl3/igt@kms_flip@flip-vs-suspend@c-dp1.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl8/igt@kms_flip@flip-vs-suspend@c-dp1.html

  * igt@kms_flip@plain-flip-ts-check@b-hdmi-a1:
    - shard-glk:          [PASS][67] -> [FAIL][68] ([i915#2122])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-glk2/igt@kms_flip@plain-flip-ts-check@b-hdmi-a1.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk3/igt@kms_flip@plain-flip-ts-check@b-hdmi-a1.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-kbl:          NOTRUN -> [SKIP][69] ([fdo#109271]) +46 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-cur-indfb-draw-mmap-wc.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-indfb-draw-mmap-wc:
    - shard-iclb:         NOTRUN -> [SKIP][70] ([fdo#109280]) +5 similar issues
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-indfb-draw-mmap-wc.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt:
    - shard-apl:          NOTRUN -> [SKIP][71] ([fdo#109271]) +91 similar issues
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-d-frame-sequence:
    - shard-skl:          NOTRUN -> [SKIP][72] ([fdo#109271] / [i915#533])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@kms_pipe_crc_basic@read-crc-pipe-d-frame-sequence.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [PASS][73] -> [DMESG-WARN][74] ([i915#180]) +2 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-kbl4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-apl:          NOTRUN -> [FAIL][75] ([fdo#108145] / [i915#265])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl1/igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-skl:          NOTRUN -> [FAIL][76] ([fdo#108145] / [i915#265]) +1 similar issue
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max.html

  * igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb:
    - shard-glk:          NOTRUN -> [FAIL][77] ([i915#265])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb.html
    - shard-apl:          NOTRUN -> [FAIL][78] ([i915#265]) +1 similar issue
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb:
    - shard-skl:          NOTRUN -> [FAIL][79] ([i915#265])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html

  * igt@kms_psr2_su@frontbuffer-xrgb8888:
    - shard-glk:          NOTRUN -> [SKIP][80] ([fdo#109271] / [i915#658]) +1 similar issue
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk8/igt@kms_psr2_su@frontbuffer-xrgb8888.html
    - shard-apl:          NOTRUN -> [SKIP][81] ([fdo#109271] / [i915#658])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_psr2_su@frontbuffer-xrgb8888.html

  * igt@kms_psr2_su@page_flip-p010:
    - shard-skl:          NOTRUN -> [SKIP][82] ([fdo#109271] / [i915#658])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl9/igt@kms_psr2_su@page_flip-p010.html

  * igt@kms_setmode@basic:
    - shard-apl:          NOTRUN -> [FAIL][83] ([i915#31])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl2/igt@kms_setmode@basic.html

  * igt@kms_sysfs_edid_timing:
    - shard-skl:          NOTRUN -> [FAIL][84] ([IGT#2])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl8/igt@kms_sysfs_edid_timing.html

  * igt@kms_tv_load_detect@load-detect:
    - shard-tglb:         NOTRUN -> [SKIP][85] ([fdo#109309])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb3/igt@kms_tv_load_detect@load-detect.html

  * igt@kms_vblank@pipe-c-accuracy-idle:
    - shard-skl:          [PASS][86] -> [FAIL][87] ([i915#43])
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl3/igt@kms_vblank@pipe-c-accuracy-idle.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl7/igt@kms_vblank@pipe-c-accuracy-idle.html

  * igt@kms_writeback@writeback-pixel-formats:
    - shard-apl:          NOTRUN -> [SKIP][88] ([fdo#109271] / [i915#2437])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl1/igt@kms_writeback@writeback-pixel-formats.html

  * igt@sysfs_clients@create:
    - shard-iclb:         NOTRUN -> [SKIP][89] ([i915#2994])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb8/igt@sysfs_clients@create.html

  * igt@sysfs_clients@fair-7:
    - shard-apl:          NOTRUN -> [SKIP][90] ([fdo#109271] / [i915#2994])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl1/igt@sysfs_clients@fair-7.html

  * igt@sysfs_clients@pidname:
    - shard-skl:          NOTRUN -> [SKIP][91] ([fdo#109271] / [i915#2994])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@sysfs_clients@pidname.html

  * igt@sysfs_clients@recycle-many:
    - shard-tglb:         NOTRUN -> [SKIP][92] ([i915#2994])
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb3/igt@sysfs_clients@recycle-many.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@preservation-s3@vecs0:
    - shard-skl:          [INCOMPLETE][93] ([i915#4939]) -> [PASS][94]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl10/igt@gem_ctx_isolation@preservation-s3@vecs0.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl6/igt@gem_ctx_isolation@preservation-s3@vecs0.html

  * igt@gem_ctx_persistence@many-contexts:
    - {shard-rkl}:        [FAIL][95] ([i915#2410]) -> [PASS][96]
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@gem_ctx_persistence@many-contexts.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-2/igt@gem_ctx_persistence@many-contexts.html

  * igt@gem_eio@kms:
    - shard-tglb:         [FAIL][97] ([i915#232]) -> [PASS][98]
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglb2/igt@gem_eio@kms.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglb7/igt@gem_eio@kms.html

  * igt@gem_exec_fair@basic-deadline:
    - {shard-rkl}:        [FAIL][99] ([i915#2846]) -> [PASS][100]
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-6/igt@gem_exec_fair@basic-deadline.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-5/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][101] ([i915#2842]) -> [PASS][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-apl4/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-apl7/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - shard-glk:          [FAIL][103] ([i915#2842]) -> [PASS][104]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-glk9/igt@gem_exec_fair@basic-pace@rcs0.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk9/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-iclb:         [FAIL][105] ([i915#2842]) -> [PASS][106]
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-iclb3/igt@gem_exec_fair@basic-pace@vecs0.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-iclb7/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@gem_exec_schedule@deep@rcs0:
    - {shard-rkl}:        [INCOMPLETE][107] ([i915#5080]) -> [PASS][108]
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@gem_exec_schedule@deep@rcs0.html
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-5/igt@gem_exec_schedule@deep@rcs0.html

  * igt@gem_exec_whisper@basic-queues-forked-all:
    - shard-glk:          [DMESG-WARN][109] ([i915#118]) -> [PASS][110] +2 similar issues
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-glk4/igt@gem_exec_whisper@basic-queues-forked-all.html
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-glk2/igt@gem_exec_whisper@basic-queues-forked-all.html

  * igt@gem_lmem_swapping@smem-oom@lmem0:
    - {shard-dg1}:        [DMESG-WARN][111] ([i915#4936]) -> [PASS][112]
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-dg1-15/igt@gem_lmem_swapping@smem-oom@lmem0.html
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-dg1-18/igt@gem_lmem_swapping@smem-oom@lmem0.html

  * igt@i915_module_load@reload-with-fault-injection:
    - shard-skl:          [DMESG-WARN][113] ([i915#1982]) -> [PASS][114] +1 similar issue
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-skl4/igt@i915_module_load@reload-with-fault-injection.html
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-skl10/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_pm_dc@dc6-psr:
    - {shard-rkl}:        [SKIP][115] ([i915#658]) -> [PASS][116]
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@i915_pm_dc@dc6-psr.html
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_pm_rpm@modeset-non-lpsp-stress:
    - {shard-dg1}:        [SKIP][117] ([i915#1397]) -> [PASS][118]
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-dg1-15/igt@i915_pm_rpm@modeset-non-lpsp-stress.html
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-dg1-18/igt@i915_pm_rpm@modeset-non-lpsp-stress.html

  * igt@kms_big_fb@linear-16bpp-rotate-180:
    - {shard-tglu}:       [DMESG-WARN][119] ([i915#402]) -> [PASS][120]
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglu-3/igt@kms_big_fb@linear-16bpp-rotate-180.html
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglu-6/igt@kms_big_fb@linear-16bpp-rotate-180.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-0:
    - {shard-tglu}:       [DMESG-WARN][121] ([i915#1982] / [i915#402]) -> [PASS][122]
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglu-2/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglu-5/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs:
    - {shard-tglu}:       [FAIL][123] ([i915#5046]) -> [PASS][124]
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-tglu-1/igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs.html
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-tglu-4/igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs.html

  * igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc:
    - {shard-rkl}:        [SKIP][125] ([i915#1845] / [i915#4098]) -> [PASS][126] +2 similar issues
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc.html
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_color@pipe-a-ctm-blue-to-red:
    - {shard-rkl}:        [SKIP][127] ([i915#1149] / [i915#1849]) -> [PASS][128]
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@kms_color@pipe-a-ctm-blue-to-red.html
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_color@pipe-a-ctm-blue-to-red.html

  * igt@kms_color@pipe-b-ctm-0-25:
    - {shard-rkl}:        [SKIP][129] ([i915#1149] / [i915#1849] / [i915#4070]) -> [PASS][130]
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-2/igt@kms_color@pipe-b-ctm-0-25.html
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_color@pipe-b-ctm-0-25.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x85-random:
    - {shard-rkl}:        [SKIP][131] ([fdo#112022] / [i915#4070]) -> [PASS][132] +2 similar issues
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-2/igt@kms_cursor_crc@pipe-a-cursor-256x85-random.html
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_cursor_crc@pipe-a-cursor-256x85-random.html

  * igt@kms_cursor_crc@pipe-a-cursor-alpha-transparent:
    - {shard-rkl}:        [SKIP][133] ([fdo#112022]) -> [PASS][134] +1 similar issue
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@kms_cursor_crc@pipe-a-cursor-alpha-transparent.html
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_cursor_crc@pipe-a-cursor-alpha-transparent.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][135] ([i915#180]) -> [PASS][136]
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_legacy@cursora-vs-flipa-atomic-transitions:
    - {shard-rkl}:        [SKIP][137] ([fdo#111825] / [i915#4070]) -> [PASS][138]
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-2/igt@kms_cursor_legacy@cursora-vs-flipa-atomic-transitions.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_cursor_legacy@cursora-vs-flipa-atomic-transitions.html

  * igt@kms_cursor_legacy@flip-vs-cursor-legacy:
    - {shard-rkl}:        [SKIP][139] ([fdo#111825]) -> [PASS][140]
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332/shard-rkl-5/igt@kms_cursor_legacy@flip-vs-cursor-legacy.html
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/shard-rkl-6/igt@kms_cursor_legacy@flip-vs-cursor-legacy.html

  * igt@kms_cursor_legacy@pipe-a-single-bo:
    - {shard-rkl}:        [INCOMPLETE][141] -> [PASS][142] +1 similar issue
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11332

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22502/index.html

[-- Attachment #2: Type: text/html, Size: 33471 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 16:41   ` [Intel-gfx] " Matthew Auld
  (?)
@ 2022-03-08 13:59     ` Arunpravin
  -1 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 13:59 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 07/03/22 10:11 pm, Matthew Auld wrote:
> On 07/03/2022 14:37, Arunpravin wrote:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
> 
> Isn't the issue that we are allowing a size that is not aligned to the 
> requested min_page_size? Should we not fix the caller(and throw a normal 
> error here), or perhaps add the round_up() here instead?
> 
CASE 1:
when size is not aligned to the requested min_page_size, for instance,
required size = 161 pages, min_page_size = 128 pages, here we have 3
possible options,
a. AFAIK,This kind of situation is common in any workload,the first
allocation (i.e) 128 pages is aligned to min_page_size, Should we just
allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
know the left over pages are not in min_page_size alignment?

b. There are many such instances in unigine heaven workload (there would
be many such workloads), throwing a normal error would lower the FPS? is
it possible to fix at caller application?

c. adding the round_up() is possible, but in every such instances we end
up allocating extra unused memory. For example, if required pages = 1028
and min_page_size = 1024 pages, we end up round up of left over 4 pages
to the min_page_size, so the total size would be 2048 pages.

> i.e if someone does:
> 
> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
CASE 2:
I think this case should be detected (i.e) when min_page_size > size,
should we return -EINVAL?
> 
> This will still trigger the BUG_ON() even if we move it out of the loop, 
> AFAICT.
> 

Should we just allow the CASE 1 proceed for the allocation and return
-EINVAL for the CASE 2?

>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 13:59     ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 13:59 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 07/03/22 10:11 pm, Matthew Auld wrote:
> On 07/03/2022 14:37, Arunpravin wrote:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
> 
> Isn't the issue that we are allowing a size that is not aligned to the 
> requested min_page_size? Should we not fix the caller(and throw a normal 
> error here), or perhaps add the round_up() here instead?
> 
CASE 1:
when size is not aligned to the requested min_page_size, for instance,
required size = 161 pages, min_page_size = 128 pages, here we have 3
possible options,
a. AFAIK,This kind of situation is common in any workload,the first
allocation (i.e) 128 pages is aligned to min_page_size, Should we just
allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
know the left over pages are not in min_page_size alignment?

b. There are many such instances in unigine heaven workload (there would
be many such workloads), throwing a normal error would lower the FPS? is
it possible to fix at caller application?

c. adding the round_up() is possible, but in every such instances we end
up allocating extra unused memory. For example, if required pages = 1028
and min_page_size = 1024 pages, we end up round up of left over 4 pages
to the min_page_size, so the total size would be 2048 pages.

> i.e if someone does:
> 
> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
CASE 2:
I think this case should be detected (i.e) when min_page_size > size,
should we return -EINVAL?
> 
> This will still trigger the BUG_ON() even if we move it out of the loop, 
> AFAICT.
> 

Should we just allow the CASE 1 proceed for the allocation and return
-EINVAL for the CASE 2?

>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 13:59     ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 13:59 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel



On 07/03/22 10:11 pm, Matthew Auld wrote:
> On 07/03/2022 14:37, Arunpravin wrote:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
> 
> Isn't the issue that we are allowing a size that is not aligned to the 
> requested min_page_size? Should we not fix the caller(and throw a normal 
> error here), or perhaps add the round_up() here instead?
> 
CASE 1:
when size is not aligned to the requested min_page_size, for instance,
required size = 161 pages, min_page_size = 128 pages, here we have 3
possible options,
a. AFAIK,This kind of situation is common in any workload,the first
allocation (i.e) 128 pages is aligned to min_page_size, Should we just
allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
know the left over pages are not in min_page_size alignment?

b. There are many such instances in unigine heaven workload (there would
be many such workloads), throwing a normal error would lower the FPS? is
it possible to fix at caller application?

c. adding the round_up() is possible, but in every such instances we end
up allocating extra unused memory. For example, if required pages = 1028
and min_page_size = 1024 pages, we end up round up of left over 4 pages
to the min_page_size, so the total size would be 2048 pages.

> i.e if someone does:
> 
> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
CASE 2:
I think this case should be detected (i.e) when min_page_size > size,
should we return -EINVAL?
> 
> This will still trigger the BUG_ON() even if we move it out of the loop, 
> AFAICT.
> 

Should we just allow the CASE 1 proceed for the allocation and return
-EINVAL for the CASE 2?

>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 15:53   ` [Intel-gfx] " Christian König
  (?)
@ 2022-03-08 14:27     ` Arunpravin
  -1 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 14:27 UTC (permalink / raw)
  To: Christian König, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, matthew.auld



On 07/03/22 9:23 pm, Christian König wrote:
> Am 07.03.22 um 15:37 schrieb Arunpravin:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
> 
> Well using BUG_ON sounds like the wrong approach in the first place.
> 
> A BUG_ON() is only justified if you prevent further data corruption, 
> e.g. when you detect for example a reference count overflow or similar.
> 
> In all other cases you should trigger a WARN_ON() and abort the 
> operation with -EINVAL if possible.
> 
> Regards,
> Christian.
> 
ok, in this case, I think it is acceptable to use WARN_ON and abort
using -EINVAL

Regards,
Arun
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 14:27     ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 14:27 UTC (permalink / raw)
  To: Christian König, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, matthew.auld



On 07/03/22 9:23 pm, Christian König wrote:
> Am 07.03.22 um 15:37 schrieb Arunpravin:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
> 
> Well using BUG_ON sounds like the wrong approach in the first place.
> 
> A BUG_ON() is only justified if you prevent further data corruption, 
> e.g. when you detect for example a reference count overflow or similar.
> 
> In all other cases you should trigger a WARN_ON() and abort the 
> operation with -EINVAL if possible.
> 
> Regards,
> Christian.
> 
ok, in this case, I think it is acceptable to use WARN_ON and abort
using -EINVAL

Regards,
Arun
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 14:27     ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 14:27 UTC (permalink / raw)
  To: Christian König, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, matthew.auld, daniel



On 07/03/22 9:23 pm, Christian König wrote:
> Am 07.03.22 um 15:37 schrieb Arunpravin:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
> 
> Well using BUG_ON sounds like the wrong approach in the first place.
> 
> A BUG_ON() is only justified if you prevent further data corruption, 
> e.g. when you detect for example a reference count overflow or similar.
> 
> In all other cases you should trigger a WARN_ON() and abort the 
> operation with -EINVAL if possible.
> 
> Regards,
> Christian.
> 
ok, in this case, I think it is acceptable to use WARN_ON and abort
using -EINVAL

Regards,
Arun
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
  2022-03-07 14:45 ` Jani Nikula
@ 2022-03-08 14:32   ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-08 14:32 UTC (permalink / raw)
  To: Jani Nikula, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, matthew.auld



On 07/03/22 8:15 pm, Jani Nikula wrote:
> On Mon, 07 Mar 2022, Arunpravin <Arunpravin.PaneerSelvam@amd.com> wrote:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
> 
> How about turning these BUG_ON()s to WARN_ON()s with an error return?
> What's the point in oopsing?
> 
yes, we will use WARN_ON with an error return

Thanks,
Arun
> BR,
> Jani.
> 
> 
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>  drivers/gpu/drm/drm_buddy.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>  	order = fls(pages) - 1;
>>  	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>  
>> +	BUG_ON(order < min_order);
>> +
>>  	do {
>>  		order = min(order, (unsigned int)fls(pages) - 1);
>>  		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>  
>>  		do {
>>  			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-08 13:59     ` [Intel-gfx] " Arunpravin
  (?)
@ 2022-03-08 17:01       ` Matthew Auld
  -1 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-08 17:01 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 08/03/2022 13:59, Arunpravin wrote:
> 
> 
> On 07/03/22 10:11 pm, Matthew Auld wrote:
>> On 07/03/2022 14:37, Arunpravin wrote:
>>> place BUG_ON(order < min_order) outside do..while
>>> loop as it fails Unigine Heaven benchmark.
>>>
>>> Unigine Heaven has buffer allocation requests for
>>> example required pages are 161 and alignment request
>>> is 128. To allocate the remaining 33 pages, continues
>>> the iteration to find the order value which is 5 and
>>> when it compares with min_order = 7, enables the
>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>> check outside of do..while loop.
>>>
>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>> index 72f52f293249..ed94c56b720f 100644
>>> --- a/drivers/gpu/drm/drm_buddy.c
>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>    	order = fls(pages) - 1;
>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>    
>>> +	BUG_ON(order < min_order);
>>
>> Isn't the issue that we are allowing a size that is not aligned to the
>> requested min_page_size? Should we not fix the caller(and throw a normal
>> error here), or perhaps add the round_up() here instead?
>>
> CASE 1:
> when size is not aligned to the requested min_page_size, for instance,
> required size = 161 pages, min_page_size = 128 pages, here we have 3
> possible options,
> a. AFAIK,This kind of situation is common in any workload,the first
> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
> know the left over pages are not in min_page_size alignment?

So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
arbitrary physical alignment for an object? Is that not meant to apply 
to every page/chunk? The above example would only have the correct 
physical alignment guaranteed for the first chunk, or so, is this the 
expected ABI behaviour?

Also looking at this some more, the other related bug here is the 
order-- == min_order check, since it now won't bail when order == 0, 
leading to order = -1, if we are unlucky...

Originally, if asking for min_page_size > chunk_size, then the 
allocation was meant to fail if it can't fill the resource request with 
pages of at least that size(and also alignment). Or at least that was 
the original meaning in i915 IIRC.

> 
> b. There are many such instances in unigine heaven workload (there would
> be many such workloads), throwing a normal error would lower the FPS? is
> it possible to fix at caller application?
> 
> c. adding the round_up() is possible, but in every such instances we end
> up allocating extra unused memory. For example, if required pages = 1028
> and min_page_size = 1024 pages, we end up round up of left over 4 pages
> to the min_page_size, so the total size would be 2048 pages.
> 
>> i.e if someone does:
>>
>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
> CASE 2:
> I think this case should be detected (i.e) when min_page_size > size,
> should we return -EINVAL?
>>
>> This will still trigger the BUG_ON() even if we move it out of the loop,
>> AFAICT.
>>
> 
> Should we just allow the CASE 1 proceed for the allocation and return
> -EINVAL for the CASE 2?
> 
>>> +
>>>    	do {
>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>    		BUG_ON(order > mm->max_order);
>>> -		BUG_ON(order < min_order);
>>>    
>>>    		do {
>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>
>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 17:01       ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-08 17:01 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 08/03/2022 13:59, Arunpravin wrote:
> 
> 
> On 07/03/22 10:11 pm, Matthew Auld wrote:
>> On 07/03/2022 14:37, Arunpravin wrote:
>>> place BUG_ON(order < min_order) outside do..while
>>> loop as it fails Unigine Heaven benchmark.
>>>
>>> Unigine Heaven has buffer allocation requests for
>>> example required pages are 161 and alignment request
>>> is 128. To allocate the remaining 33 pages, continues
>>> the iteration to find the order value which is 5 and
>>> when it compares with min_order = 7, enables the
>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>> check outside of do..while loop.
>>>
>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>> index 72f52f293249..ed94c56b720f 100644
>>> --- a/drivers/gpu/drm/drm_buddy.c
>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>    	order = fls(pages) - 1;
>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>    
>>> +	BUG_ON(order < min_order);
>>
>> Isn't the issue that we are allowing a size that is not aligned to the
>> requested min_page_size? Should we not fix the caller(and throw a normal
>> error here), or perhaps add the round_up() here instead?
>>
> CASE 1:
> when size is not aligned to the requested min_page_size, for instance,
> required size = 161 pages, min_page_size = 128 pages, here we have 3
> possible options,
> a. AFAIK,This kind of situation is common in any workload,the first
> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
> know the left over pages are not in min_page_size alignment?

So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
arbitrary physical alignment for an object? Is that not meant to apply 
to every page/chunk? The above example would only have the correct 
physical alignment guaranteed for the first chunk, or so, is this the 
expected ABI behaviour?

Also looking at this some more, the other related bug here is the 
order-- == min_order check, since it now won't bail when order == 0, 
leading to order = -1, if we are unlucky...

Originally, if asking for min_page_size > chunk_size, then the 
allocation was meant to fail if it can't fill the resource request with 
pages of at least that size(and also alignment). Or at least that was 
the original meaning in i915 IIRC.

> 
> b. There are many such instances in unigine heaven workload (there would
> be many such workloads), throwing a normal error would lower the FPS? is
> it possible to fix at caller application?
> 
> c. adding the round_up() is possible, but in every such instances we end
> up allocating extra unused memory. For example, if required pages = 1028
> and min_page_size = 1024 pages, we end up round up of left over 4 pages
> to the min_page_size, so the total size would be 2048 pages.
> 
>> i.e if someone does:
>>
>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
> CASE 2:
> I think this case should be detected (i.e) when min_page_size > size,
> should we return -EINVAL?
>>
>> This will still trigger the BUG_ON() even if we move it out of the loop,
>> AFAICT.
>>
> 
> Should we just allow the CASE 1 proceed for the allocation and return
> -EINVAL for the CASE 2?
> 
>>> +
>>>    	do {
>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>    		BUG_ON(order > mm->max_order);
>>> -		BUG_ON(order < min_order);
>>>    
>>>    		do {
>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>
>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-08 17:01       ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-08 17:01 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel

On 08/03/2022 13:59, Arunpravin wrote:
> 
> 
> On 07/03/22 10:11 pm, Matthew Auld wrote:
>> On 07/03/2022 14:37, Arunpravin wrote:
>>> place BUG_ON(order < min_order) outside do..while
>>> loop as it fails Unigine Heaven benchmark.
>>>
>>> Unigine Heaven has buffer allocation requests for
>>> example required pages are 161 and alignment request
>>> is 128. To allocate the remaining 33 pages, continues
>>> the iteration to find the order value which is 5 and
>>> when it compares with min_order = 7, enables the
>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>> check outside of do..while loop.
>>>
>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>> index 72f52f293249..ed94c56b720f 100644
>>> --- a/drivers/gpu/drm/drm_buddy.c
>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>    	order = fls(pages) - 1;
>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>    
>>> +	BUG_ON(order < min_order);
>>
>> Isn't the issue that we are allowing a size that is not aligned to the
>> requested min_page_size? Should we not fix the caller(and throw a normal
>> error here), or perhaps add the round_up() here instead?
>>
> CASE 1:
> when size is not aligned to the requested min_page_size, for instance,
> required size = 161 pages, min_page_size = 128 pages, here we have 3
> possible options,
> a. AFAIK,This kind of situation is common in any workload,the first
> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
> know the left over pages are not in min_page_size alignment?

So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
arbitrary physical alignment for an object? Is that not meant to apply 
to every page/chunk? The above example would only have the correct 
physical alignment guaranteed for the first chunk, or so, is this the 
expected ABI behaviour?

Also looking at this some more, the other related bug here is the 
order-- == min_order check, since it now won't bail when order == 0, 
leading to order = -1, if we are unlucky...

Originally, if asking for min_page_size > chunk_size, then the 
allocation was meant to fail if it can't fill the resource request with 
pages of at least that size(and also alignment). Or at least that was 
the original meaning in i915 IIRC.

> 
> b. There are many such instances in unigine heaven workload (there would
> be many such workloads), throwing a normal error would lower the FPS? is
> it possible to fix at caller application?
> 
> c. adding the round_up() is possible, but in every such instances we end
> up allocating extra unused memory. For example, if required pages = 1028
> and min_page_size = 1024 pages, we end up round up of left over 4 pages
> to the min_page_size, so the total size would be 2048 pages.
> 
>> i.e if someone does:
>>
>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
> CASE 2:
> I think this case should be detected (i.e) when min_page_size > size,
> should we return -EINVAL?
>>
>> This will still trigger the BUG_ON() even if we move it out of the loop,
>> AFAICT.
>>
> 
> Should we just allow the CASE 1 proceed for the allocation and return
> -EINVAL for the CASE 2?
> 
>>> +
>>>    	do {
>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>    		BUG_ON(order > mm->max_order);
>>> -		BUG_ON(order < min_order);
>>>    
>>>    		do {
>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>
>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-08 17:01       ` [Intel-gfx] " Matthew Auld
  (?)
@ 2022-03-10 14:47         ` Arunpravin
  -1 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-10 14:47 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 08/03/22 10:31 pm, Matthew Auld wrote:
> On 08/03/2022 13:59, Arunpravin wrote:
>>
>>
>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>> place BUG_ON(order < min_order) outside do..while
>>>> loop as it fails Unigine Heaven benchmark.
>>>>
>>>> Unigine Heaven has buffer allocation requests for
>>>> example required pages are 161 and alignment request
>>>> is 128. To allocate the remaining 33 pages, continues
>>>> the iteration to find the order value which is 5 and
>>>> when it compares with min_order = 7, enables the
>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>> check outside of do..while loop.
>>>>
>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>> index 72f52f293249..ed94c56b720f 100644
>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>    	order = fls(pages) - 1;
>>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>    
>>>> +	BUG_ON(order < min_order);
>>>
>>> Isn't the issue that we are allowing a size that is not aligned to the
>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>> error here), or perhaps add the round_up() here instead?
>>>
>> CASE 1:
>> when size is not aligned to the requested min_page_size, for instance,
>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>> possible options,
>> a. AFAIK,This kind of situation is common in any workload,the first
>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>> know the left over pages are not in min_page_size alignment?
> 
> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
> arbitrary physical alignment for an object? Is that not meant to apply 
> to every page/chunk? The above example would only have the correct 
> physical alignment guaranteed for the first chunk, or so, is this the 
> expected ABI behaviour?
> 
I gone through the function amdgpu_gem_create_ioctl(), it reads the
physical alignment in bytes from userspace, does i915 round up the size
value to the alignment or does i915 fails the allocation request if size
is not aligned with min_page_size? If not, I think running unigine
heaven or similar benchmark triggers BUG_ON() on current version of drm
buddy
> Also looking at this some more, the other related bug here is the 
> order-- == min_order check, since it now won't bail when order == 0, 
> leading to order = -1, if we are unlucky...
will add a fix
> 
> Originally, if asking for min_page_size > chunk_size, then the 
> allocation was meant to fail if it can't fill the resource request with 
> pages of at least that size(and also alignment). Or at least that was 
> the original meaning in i915 IIRC.
we can follow the same here too, failing the allocation request if size
is not aligned with min_page_size?

I added a debug print for requested num_pages from userspace and its
alignment request and executed unigine heaven, I see many such instances
where min_page_size is not aligned to the size, how i915 handles such
requests?
> 
>>
>> b. There are many such instances in unigine heaven workload (there would
>> be many such workloads), throwing a normal error would lower the FPS? is
>> it possible to fix at caller application?
>>
>> c. adding the round_up() is possible, but in every such instances we end
>> up allocating extra unused memory. For example, if required pages = 1028
>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>> to the min_page_size, so the total size would be 2048 pages.
>>
>>> i.e if someone does:
>>>
>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>> CASE 2:
>> I think this case should be detected (i.e) when min_page_size > size,
>> should we return -EINVAL?
>>>
>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>> AFAICT.
>>>
>>
>> Should we just allow the CASE 1 proceed for the allocation and return
>> -EINVAL for the CASE 2?
>>
>>>> +
>>>>    	do {
>>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>>    		BUG_ON(order > mm->max_order);
>>>> -		BUG_ON(order < min_order);
>>>>    
>>>>    		do {
>>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>
>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-10 14:47         ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-10 14:47 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 08/03/22 10:31 pm, Matthew Auld wrote:
> On 08/03/2022 13:59, Arunpravin wrote:
>>
>>
>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>> place BUG_ON(order < min_order) outside do..while
>>>> loop as it fails Unigine Heaven benchmark.
>>>>
>>>> Unigine Heaven has buffer allocation requests for
>>>> example required pages are 161 and alignment request
>>>> is 128. To allocate the remaining 33 pages, continues
>>>> the iteration to find the order value which is 5 and
>>>> when it compares with min_order = 7, enables the
>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>> check outside of do..while loop.
>>>>
>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>> index 72f52f293249..ed94c56b720f 100644
>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>    	order = fls(pages) - 1;
>>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>    
>>>> +	BUG_ON(order < min_order);
>>>
>>> Isn't the issue that we are allowing a size that is not aligned to the
>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>> error here), or perhaps add the round_up() here instead?
>>>
>> CASE 1:
>> when size is not aligned to the requested min_page_size, for instance,
>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>> possible options,
>> a. AFAIK,This kind of situation is common in any workload,the first
>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>> know the left over pages are not in min_page_size alignment?
> 
> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
> arbitrary physical alignment for an object? Is that not meant to apply 
> to every page/chunk? The above example would only have the correct 
> physical alignment guaranteed for the first chunk, or so, is this the 
> expected ABI behaviour?
> 
I gone through the function amdgpu_gem_create_ioctl(), it reads the
physical alignment in bytes from userspace, does i915 round up the size
value to the alignment or does i915 fails the allocation request if size
is not aligned with min_page_size? If not, I think running unigine
heaven or similar benchmark triggers BUG_ON() on current version of drm
buddy
> Also looking at this some more, the other related bug here is the 
> order-- == min_order check, since it now won't bail when order == 0, 
> leading to order = -1, if we are unlucky...
will add a fix
> 
> Originally, if asking for min_page_size > chunk_size, then the 
> allocation was meant to fail if it can't fill the resource request with 
> pages of at least that size(and also alignment). Or at least that was 
> the original meaning in i915 IIRC.
we can follow the same here too, failing the allocation request if size
is not aligned with min_page_size?

I added a debug print for requested num_pages from userspace and its
alignment request and executed unigine heaven, I see many such instances
where min_page_size is not aligned to the size, how i915 handles such
requests?
> 
>>
>> b. There are many such instances in unigine heaven workload (there would
>> be many such workloads), throwing a normal error would lower the FPS? is
>> it possible to fix at caller application?
>>
>> c. adding the round_up() is possible, but in every such instances we end
>> up allocating extra unused memory. For example, if required pages = 1028
>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>> to the min_page_size, so the total size would be 2048 pages.
>>
>>> i.e if someone does:
>>>
>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>> CASE 2:
>> I think this case should be detected (i.e) when min_page_size > size,
>> should we return -EINVAL?
>>>
>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>> AFAICT.
>>>
>>
>> Should we just allow the CASE 1 proceed for the allocation and return
>> -EINVAL for the CASE 2?
>>
>>>> +
>>>>    	do {
>>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>>    		BUG_ON(order > mm->max_order);
>>>> -		BUG_ON(order < min_order);
>>>>    
>>>>    		do {
>>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>
>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-10 14:47         ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-10 14:47 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel



On 08/03/22 10:31 pm, Matthew Auld wrote:
> On 08/03/2022 13:59, Arunpravin wrote:
>>
>>
>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>> place BUG_ON(order < min_order) outside do..while
>>>> loop as it fails Unigine Heaven benchmark.
>>>>
>>>> Unigine Heaven has buffer allocation requests for
>>>> example required pages are 161 and alignment request
>>>> is 128. To allocate the remaining 33 pages, continues
>>>> the iteration to find the order value which is 5 and
>>>> when it compares with min_order = 7, enables the
>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>> check outside of do..while loop.
>>>>
>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>> index 72f52f293249..ed94c56b720f 100644
>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>    	order = fls(pages) - 1;
>>>>    	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>    
>>>> +	BUG_ON(order < min_order);
>>>
>>> Isn't the issue that we are allowing a size that is not aligned to the
>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>> error here), or perhaps add the round_up() here instead?
>>>
>> CASE 1:
>> when size is not aligned to the requested min_page_size, for instance,
>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>> possible options,
>> a. AFAIK,This kind of situation is common in any workload,the first
>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>> know the left over pages are not in min_page_size alignment?
> 
> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some 
> arbitrary physical alignment for an object? Is that not meant to apply 
> to every page/chunk? The above example would only have the correct 
> physical alignment guaranteed for the first chunk, or so, is this the 
> expected ABI behaviour?
> 
I gone through the function amdgpu_gem_create_ioctl(), it reads the
physical alignment in bytes from userspace, does i915 round up the size
value to the alignment or does i915 fails the allocation request if size
is not aligned with min_page_size? If not, I think running unigine
heaven or similar benchmark triggers BUG_ON() on current version of drm
buddy
> Also looking at this some more, the other related bug here is the 
> order-- == min_order check, since it now won't bail when order == 0, 
> leading to order = -1, if we are unlucky...
will add a fix
> 
> Originally, if asking for min_page_size > chunk_size, then the 
> allocation was meant to fail if it can't fill the resource request with 
> pages of at least that size(and also alignment). Or at least that was 
> the original meaning in i915 IIRC.
we can follow the same here too, failing the allocation request if size
is not aligned with min_page_size?

I added a debug print for requested num_pages from userspace and its
alignment request and executed unigine heaven, I see many such instances
where min_page_size is not aligned to the size, how i915 handles such
requests?
> 
>>
>> b. There are many such instances in unigine heaven workload (there would
>> be many such workloads), throwing a normal error would lower the FPS? is
>> it possible to fix at caller application?
>>
>> c. adding the round_up() is possible, but in every such instances we end
>> up allocating extra unused memory. For example, if required pages = 1028
>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>> to the min_page_size, so the total size would be 2048 pages.
>>
>>> i.e if someone does:
>>>
>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>> CASE 2:
>> I think this case should be detected (i.e) when min_page_size > size,
>> should we return -EINVAL?
>>>
>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>> AFAICT.
>>>
>>
>> Should we just allow the CASE 1 proceed for the allocation and return
>> -EINVAL for the CASE 2?
>>
>>>> +
>>>>    	do {
>>>>    		order = min(order, (unsigned int)fls(pages) - 1);
>>>>    		BUG_ON(order > mm->max_order);
>>>> -		BUG_ON(order < min_order);
>>>>    
>>>>    		do {
>>>>    			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>
>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-10 14:47         ` [Intel-gfx] " Arunpravin
  (?)
@ 2022-03-10 15:29           ` Matthew Auld
  -1 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-10 15:29 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 10/03/2022 14:47, Arunpravin wrote:
> 
> 
> On 08/03/22 10:31 pm, Matthew Auld wrote:
>> On 08/03/2022 13:59, Arunpravin wrote:
>>>
>>>
>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>> place BUG_ON(order < min_order) outside do..while
>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>
>>>>> Unigine Heaven has buffer allocation requests for
>>>>> example required pages are 161 and alignment request
>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>> the iteration to find the order value which is 5 and
>>>>> when it compares with min_order = 7, enables the
>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>> check outside of do..while loop.
>>>>>
>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>     	order = fls(pages) - 1;
>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>     
>>>>> +	BUG_ON(order < min_order);
>>>>
>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>> error here), or perhaps add the round_up() here instead?
>>>>
>>> CASE 1:
>>> when size is not aligned to the requested min_page_size, for instance,
>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>> possible options,
>>> a. AFAIK,This kind of situation is common in any workload,the first
>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>> know the left over pages are not in min_page_size alignment?
>>
>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>> arbitrary physical alignment for an object? Is that not meant to apply
>> to every page/chunk? The above example would only have the correct
>> physical alignment guaranteed for the first chunk, or so, is this the
>> expected ABI behaviour?
>>
> I gone through the function amdgpu_gem_create_ioctl(), it reads the
> physical alignment in bytes from userspace, does i915 round up the size
> value to the alignment or does i915 fails the allocation request if size
> is not aligned with min_page_size? If not, I think running unigine
> heaven or similar benchmark triggers BUG_ON() on current version of drm
> buddy

i915 will always round_up the obj->base.size as per the 
default_page_size. But in our case the default_page_size is selected by 
the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
due to the HW having some minimum GPU page-size for mapping VRAM pages. 
We don't currently have anything similar to 
amdgpu_gem_create_in.alignment, where userspace can request some 
arbitrary physical alignment.

>> Also looking at this some more, the other related bug here is the
>> order-- == min_order check, since it now won't bail when order == 0,
>> leading to order = -1, if we are unlucky...
> will add a fix
>>
>> Originally, if asking for min_page_size > chunk_size, then the
>> allocation was meant to fail if it can't fill the resource request with
>> pages of at least that size(and also alignment). Or at least that was
>> the original meaning in i915 IIRC.
> we can follow the same here too, failing the allocation request if size
> is not aligned with min_page_size?

Yeah, seems reasonable to me.

> 
> I added a debug print for requested num_pages from userspace and its
> alignment request and executed unigine heaven, I see many such instances
> where min_page_size is not aligned to the size, how i915 handles such
> requests?
>>
>>>
>>> b. There are many such instances in unigine heaven workload (there would
>>> be many such workloads), throwing a normal error would lower the FPS? is
>>> it possible to fix at caller application?
>>>
>>> c. adding the round_up() is possible, but in every such instances we end
>>> up allocating extra unused memory. For example, if required pages = 1028
>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>> to the min_page_size, so the total size would be 2048 pages.
>>>
>>>> i.e if someone does:
>>>>
>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>> CASE 2:
>>> I think this case should be detected (i.e) when min_page_size > size,
>>> should we return -EINVAL?
>>>>
>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>> AFAICT.
>>>>
>>>
>>> Should we just allow the CASE 1 proceed for the allocation and return
>>> -EINVAL for the CASE 2?
>>>
>>>>> +
>>>>>     	do {
>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>     		BUG_ON(order > mm->max_order);
>>>>> -		BUG_ON(order < min_order);
>>>>>     
>>>>>     		do {
>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>
>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-10 15:29           ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-10 15:29 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig

On 10/03/2022 14:47, Arunpravin wrote:
> 
> 
> On 08/03/22 10:31 pm, Matthew Auld wrote:
>> On 08/03/2022 13:59, Arunpravin wrote:
>>>
>>>
>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>> place BUG_ON(order < min_order) outside do..while
>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>
>>>>> Unigine Heaven has buffer allocation requests for
>>>>> example required pages are 161 and alignment request
>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>> the iteration to find the order value which is 5 and
>>>>> when it compares with min_order = 7, enables the
>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>> check outside of do..while loop.
>>>>>
>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>     	order = fls(pages) - 1;
>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>     
>>>>> +	BUG_ON(order < min_order);
>>>>
>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>> error here), or perhaps add the round_up() here instead?
>>>>
>>> CASE 1:
>>> when size is not aligned to the requested min_page_size, for instance,
>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>> possible options,
>>> a. AFAIK,This kind of situation is common in any workload,the first
>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>> know the left over pages are not in min_page_size alignment?
>>
>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>> arbitrary physical alignment for an object? Is that not meant to apply
>> to every page/chunk? The above example would only have the correct
>> physical alignment guaranteed for the first chunk, or so, is this the
>> expected ABI behaviour?
>>
> I gone through the function amdgpu_gem_create_ioctl(), it reads the
> physical alignment in bytes from userspace, does i915 round up the size
> value to the alignment or does i915 fails the allocation request if size
> is not aligned with min_page_size? If not, I think running unigine
> heaven or similar benchmark triggers BUG_ON() on current version of drm
> buddy

i915 will always round_up the obj->base.size as per the 
default_page_size. But in our case the default_page_size is selected by 
the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
due to the HW having some minimum GPU page-size for mapping VRAM pages. 
We don't currently have anything similar to 
amdgpu_gem_create_in.alignment, where userspace can request some 
arbitrary physical alignment.

>> Also looking at this some more, the other related bug here is the
>> order-- == min_order check, since it now won't bail when order == 0,
>> leading to order = -1, if we are unlucky...
> will add a fix
>>
>> Originally, if asking for min_page_size > chunk_size, then the
>> allocation was meant to fail if it can't fill the resource request with
>> pages of at least that size(and also alignment). Or at least that was
>> the original meaning in i915 IIRC.
> we can follow the same here too, failing the allocation request if size
> is not aligned with min_page_size?

Yeah, seems reasonable to me.

> 
> I added a debug print for requested num_pages from userspace and its
> alignment request and executed unigine heaven, I see many such instances
> where min_page_size is not aligned to the size, how i915 handles such
> requests?
>>
>>>
>>> b. There are many such instances in unigine heaven workload (there would
>>> be many such workloads), throwing a normal error would lower the FPS? is
>>> it possible to fix at caller application?
>>>
>>> c. adding the round_up() is possible, but in every such instances we end
>>> up allocating extra unused memory. For example, if required pages = 1028
>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>> to the min_page_size, so the total size would be 2048 pages.
>>>
>>>> i.e if someone does:
>>>>
>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>> CASE 2:
>>> I think this case should be detected (i.e) when min_page_size > size,
>>> should we return -EINVAL?
>>>>
>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>> AFAICT.
>>>>
>>>
>>> Should we just allow the CASE 1 proceed for the allocation and return
>>> -EINVAL for the CASE 2?
>>>
>>>>> +
>>>>>     	do {
>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>     		BUG_ON(order > mm->max_order);
>>>>> -		BUG_ON(order < min_order);
>>>>>     
>>>>>     		do {
>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>
>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-10 15:29           ` Matthew Auld
  0 siblings, 0 replies; 32+ messages in thread
From: Matthew Auld @ 2022-03-10 15:29 UTC (permalink / raw)
  To: Arunpravin, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel

On 10/03/2022 14:47, Arunpravin wrote:
> 
> 
> On 08/03/22 10:31 pm, Matthew Auld wrote:
>> On 08/03/2022 13:59, Arunpravin wrote:
>>>
>>>
>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>> place BUG_ON(order < min_order) outside do..while
>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>
>>>>> Unigine Heaven has buffer allocation requests for
>>>>> example required pages are 161 and alignment request
>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>> the iteration to find the order value which is 5 and
>>>>> when it compares with min_order = 7, enables the
>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>> check outside of do..while loop.
>>>>>
>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>     	order = fls(pages) - 1;
>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>     
>>>>> +	BUG_ON(order < min_order);
>>>>
>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>> error here), or perhaps add the round_up() here instead?
>>>>
>>> CASE 1:
>>> when size is not aligned to the requested min_page_size, for instance,
>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>> possible options,
>>> a. AFAIK,This kind of situation is common in any workload,the first
>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>> know the left over pages are not in min_page_size alignment?
>>
>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>> arbitrary physical alignment for an object? Is that not meant to apply
>> to every page/chunk? The above example would only have the correct
>> physical alignment guaranteed for the first chunk, or so, is this the
>> expected ABI behaviour?
>>
> I gone through the function amdgpu_gem_create_ioctl(), it reads the
> physical alignment in bytes from userspace, does i915 round up the size
> value to the alignment or does i915 fails the allocation request if size
> is not aligned with min_page_size? If not, I think running unigine
> heaven or similar benchmark triggers BUG_ON() on current version of drm
> buddy

i915 will always round_up the obj->base.size as per the 
default_page_size. But in our case the default_page_size is selected by 
the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
due to the HW having some minimum GPU page-size for mapping VRAM pages. 
We don't currently have anything similar to 
amdgpu_gem_create_in.alignment, where userspace can request some 
arbitrary physical alignment.

>> Also looking at this some more, the other related bug here is the
>> order-- == min_order check, since it now won't bail when order == 0,
>> leading to order = -1, if we are unlucky...
> will add a fix
>>
>> Originally, if asking for min_page_size > chunk_size, then the
>> allocation was meant to fail if it can't fill the resource request with
>> pages of at least that size(and also alignment). Or at least that was
>> the original meaning in i915 IIRC.
> we can follow the same here too, failing the allocation request if size
> is not aligned with min_page_size?

Yeah, seems reasonable to me.

> 
> I added a debug print for requested num_pages from userspace and its
> alignment request and executed unigine heaven, I see many such instances
> where min_page_size is not aligned to the size, how i915 handles such
> requests?
>>
>>>
>>> b. There are many such instances in unigine heaven workload (there would
>>> be many such workloads), throwing a normal error would lower the FPS? is
>>> it possible to fix at caller application?
>>>
>>> c. adding the round_up() is possible, but in every such instances we end
>>> up allocating extra unused memory. For example, if required pages = 1028
>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>> to the min_page_size, so the total size would be 2048 pages.
>>>
>>>> i.e if someone does:
>>>>
>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>> CASE 2:
>>> I think this case should be detected (i.e) when min_page_size > size,
>>> should we return -EINVAL?
>>>>
>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>> AFAICT.
>>>>
>>>
>>> Should we just allow the CASE 1 proceed for the allocation and return
>>> -EINVAL for the CASE 2?
>>>
>>>>> +
>>>>>     	do {
>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>     		BUG_ON(order > mm->max_order);
>>>>> -		BUG_ON(order < min_order);
>>>>>     
>>>>>     		do {
>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>
>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
  2022-03-10 15:29           ` [Intel-gfx] " Matthew Auld
  (?)
@ 2022-03-14 19:38             ` Arunpravin
  -1 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-14 19:38 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 10/03/22 8:59 pm, Matthew Auld wrote:
> On 10/03/2022 14:47, Arunpravin wrote:
>>
>>
>> On 08/03/22 10:31 pm, Matthew Auld wrote:
>>> On 08/03/2022 13:59, Arunpravin wrote:
>>>>
>>>>
>>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>>> place BUG_ON(order < min_order) outside do..while
>>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>>
>>>>>> Unigine Heaven has buffer allocation requests for
>>>>>> example required pages are 161 and alignment request
>>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>>> the iteration to find the order value which is 5 and
>>>>>> when it compares with min_order = 7, enables the
>>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>>> check outside of do..while loop.
>>>>>>
>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>>     	order = fls(pages) - 1;
>>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>>     
>>>>>> +	BUG_ON(order < min_order);
>>>>>
>>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>>> error here), or perhaps add the round_up() here instead?
>>>>>
>>>> CASE 1:
>>>> when size is not aligned to the requested min_page_size, for instance,
>>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>>> possible options,
>>>> a. AFAIK,This kind of situation is common in any workload,the first
>>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>>> know the left over pages are not in min_page_size alignment?
>>>
>>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>>> arbitrary physical alignment for an object? Is that not meant to apply
>>> to every page/chunk? The above example would only have the correct
>>> physical alignment guaranteed for the first chunk, or so, is this the
>>> expected ABI behaviour?
>>>
>> I gone through the function amdgpu_gem_create_ioctl(), it reads the
>> physical alignment in bytes from userspace, does i915 round up the size
>> value to the alignment or does i915 fails the allocation request if size
>> is not aligned with min_page_size? If not, I think running unigine
>> heaven or similar benchmark triggers BUG_ON() on current version of drm
>> buddy
> 
> i915 will always round_up the obj->base.size as per the 
> default_page_size. But in our case the default_page_size is selected by 
> the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
> due to the HW having some minimum GPU page-size for mapping VRAM pages. 
> We don't currently have anything similar to 
> amdgpu_gem_create_in.alignment, where userspace can request some 
> arbitrary physical alignment.
> 
>>> Also looking at this some more, the other related bug here is the
>>> order-- == min_order check, since it now won't bail when order == 0,
>>> leading to order = -1, if we are unlucky...
>> will add a fix
>>>
>>> Originally, if asking for min_page_size > chunk_size, then the
>>> allocation was meant to fail if it can't fill the resource request with
>>> pages of at least that size(and also alignment). Or at least that was
>>> the original meaning in i915 IIRC.
>> we can follow the same here too, failing the allocation request if size
>> is not aligned with min_page_size?
> 
> Yeah, seems reasonable to me.
I had internal discussion with Christian and he suggested to round_up
the size to the alignment and trim the block to the required original
size. I have sent the patch, please review.

Thanks,
Arun
> 
>>
>> I added a debug print for requested num_pages from userspace and its
>> alignment request and executed unigine heaven, I see many such instances
>> where min_page_size is not aligned to the size, how i915 handles such
>> requests?
>>>
>>>>
>>>> b. There are many such instances in unigine heaven workload (there would
>>>> be many such workloads), throwing a normal error would lower the FPS? is
>>>> it possible to fix at caller application?
>>>>
>>>> c. adding the round_up() is possible, but in every such instances we end
>>>> up allocating extra unused memory. For example, if required pages = 1028
>>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>>> to the min_page_size, so the total size would be 2048 pages.
>>>>
>>>>> i.e if someone does:
>>>>>
>>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>>> CASE 2:
>>>> I think this case should be detected (i.e) when min_page_size > size,
>>>> should we return -EINVAL?
>>>>>
>>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>>> AFAICT.
>>>>>
>>>>
>>>> Should we just allow the CASE 1 proceed for the allocation and return
>>>> -EINVAL for the CASE 2?
>>>>
>>>>>> +
>>>>>>     	do {
>>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>>     		BUG_ON(order > mm->max_order);
>>>>>> -		BUG_ON(order < min_order);
>>>>>>     
>>>>>>     		do {
>>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>>
>>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-14 19:38             ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-14 19:38 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig



On 10/03/22 8:59 pm, Matthew Auld wrote:
> On 10/03/2022 14:47, Arunpravin wrote:
>>
>>
>> On 08/03/22 10:31 pm, Matthew Auld wrote:
>>> On 08/03/2022 13:59, Arunpravin wrote:
>>>>
>>>>
>>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>>> place BUG_ON(order < min_order) outside do..while
>>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>>
>>>>>> Unigine Heaven has buffer allocation requests for
>>>>>> example required pages are 161 and alignment request
>>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>>> the iteration to find the order value which is 5 and
>>>>>> when it compares with min_order = 7, enables the
>>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>>> check outside of do..while loop.
>>>>>>
>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>>     	order = fls(pages) - 1;
>>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>>     
>>>>>> +	BUG_ON(order < min_order);
>>>>>
>>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>>> error here), or perhaps add the round_up() here instead?
>>>>>
>>>> CASE 1:
>>>> when size is not aligned to the requested min_page_size, for instance,
>>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>>> possible options,
>>>> a. AFAIK,This kind of situation is common in any workload,the first
>>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>>> know the left over pages are not in min_page_size alignment?
>>>
>>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>>> arbitrary physical alignment for an object? Is that not meant to apply
>>> to every page/chunk? The above example would only have the correct
>>> physical alignment guaranteed for the first chunk, or so, is this the
>>> expected ABI behaviour?
>>>
>> I gone through the function amdgpu_gem_create_ioctl(), it reads the
>> physical alignment in bytes from userspace, does i915 round up the size
>> value to the alignment or does i915 fails the allocation request if size
>> is not aligned with min_page_size? If not, I think running unigine
>> heaven or similar benchmark triggers BUG_ON() on current version of drm
>> buddy
> 
> i915 will always round_up the obj->base.size as per the 
> default_page_size. But in our case the default_page_size is selected by 
> the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
> due to the HW having some minimum GPU page-size for mapping VRAM pages. 
> We don't currently have anything similar to 
> amdgpu_gem_create_in.alignment, where userspace can request some 
> arbitrary physical alignment.
> 
>>> Also looking at this some more, the other related bug here is the
>>> order-- == min_order check, since it now won't bail when order == 0,
>>> leading to order = -1, if we are unlucky...
>> will add a fix
>>>
>>> Originally, if asking for min_page_size > chunk_size, then the
>>> allocation was meant to fail if it can't fill the resource request with
>>> pages of at least that size(and also alignment). Or at least that was
>>> the original meaning in i915 IIRC.
>> we can follow the same here too, failing the allocation request if size
>> is not aligned with min_page_size?
> 
> Yeah, seems reasonable to me.
I had internal discussion with Christian and he suggested to round_up
the size to the alignment and trim the block to the required original
size. I have sent the patch, please review.

Thanks,
Arun
> 
>>
>> I added a debug print for requested num_pages from userspace and its
>> alignment request and executed unigine heaven, I see many such instances
>> where min_page_size is not aligned to the size, how i915 handles such
>> requests?
>>>
>>>>
>>>> b. There are many such instances in unigine heaven workload (there would
>>>> be many such workloads), throwing a normal error would lower the FPS? is
>>>> it possible to fix at caller application?
>>>>
>>>> c. adding the round_up() is possible, but in every such instances we end
>>>> up allocating extra unused memory. For example, if required pages = 1028
>>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>>> to the min_page_size, so the total size would be 2048 pages.
>>>>
>>>>> i.e if someone does:
>>>>>
>>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>>> CASE 2:
>>>> I think this case should be detected (i.e) when min_page_size > size,
>>>> should we return -EINVAL?
>>>>>
>>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>>> AFAICT.
>>>>>
>>>>
>>>> Should we just allow the CASE 1 proceed for the allocation and return
>>>> -EINVAL for the CASE 2?
>>>>
>>>>>> +
>>>>>>     	do {
>>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>>     		BUG_ON(order > mm->max_order);
>>>>>> -		BUG_ON(order < min_order);
>>>>>>     
>>>>>>     		do {
>>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>>
>>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH] drm: remove min_order BUG_ON check
@ 2022-03-14 19:38             ` Arunpravin
  0 siblings, 0 replies; 32+ messages in thread
From: Arunpravin @ 2022-03-14 19:38 UTC (permalink / raw)
  To: Matthew Auld, amd-gfx, dri-devel, intel-gfx
  Cc: alexander.deucher, christian.koenig, daniel



On 10/03/22 8:59 pm, Matthew Auld wrote:
> On 10/03/2022 14:47, Arunpravin wrote:
>>
>>
>> On 08/03/22 10:31 pm, Matthew Auld wrote:
>>> On 08/03/2022 13:59, Arunpravin wrote:
>>>>
>>>>
>>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>>> place BUG_ON(order < min_order) outside do..while
>>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>>
>>>>>> Unigine Heaven has buffer allocation requests for
>>>>>> example required pages are 161 and alignment request
>>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>>> the iteration to find the order value which is 5 and
>>>>>> when it compares with min_order = 7, enables the
>>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>>> check outside of do..while loop.
>>>>>>
>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>>     	order = fls(pages) - 1;
>>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>>     
>>>>>> +	BUG_ON(order < min_order);
>>>>>
>>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>>> error here), or perhaps add the round_up() here instead?
>>>>>
>>>> CASE 1:
>>>> when size is not aligned to the requested min_page_size, for instance,
>>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>>> possible options,
>>>> a. AFAIK,This kind of situation is common in any workload,the first
>>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>>> know the left over pages are not in min_page_size alignment?
>>>
>>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>>> arbitrary physical alignment for an object? Is that not meant to apply
>>> to every page/chunk? The above example would only have the correct
>>> physical alignment guaranteed for the first chunk, or so, is this the
>>> expected ABI behaviour?
>>>
>> I gone through the function amdgpu_gem_create_ioctl(), it reads the
>> physical alignment in bytes from userspace, does i915 round up the size
>> value to the alignment or does i915 fails the allocation request if size
>> is not aligned with min_page_size? If not, I think running unigine
>> heaven or similar benchmark triggers BUG_ON() on current version of drm
>> buddy
> 
> i915 will always round_up the obj->base.size as per the 
> default_page_size. But in our case the default_page_size is selected by 
> the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
> due to the HW having some minimum GPU page-size for mapping VRAM pages. 
> We don't currently have anything similar to 
> amdgpu_gem_create_in.alignment, where userspace can request some 
> arbitrary physical alignment.
> 
>>> Also looking at this some more, the other related bug here is the
>>> order-- == min_order check, since it now won't bail when order == 0,
>>> leading to order = -1, if we are unlucky...
>> will add a fix
>>>
>>> Originally, if asking for min_page_size > chunk_size, then the
>>> allocation was meant to fail if it can't fill the resource request with
>>> pages of at least that size(and also alignment). Or at least that was
>>> the original meaning in i915 IIRC.
>> we can follow the same here too, failing the allocation request if size
>> is not aligned with min_page_size?
> 
> Yeah, seems reasonable to me.
I had internal discussion with Christian and he suggested to round_up
the size to the alignment and trim the block to the required original
size. I have sent the patch, please review.

Thanks,
Arun
> 
>>
>> I added a debug print for requested num_pages from userspace and its
>> alignment request and executed unigine heaven, I see many such instances
>> where min_page_size is not aligned to the size, how i915 handles such
>> requests?
>>>
>>>>
>>>> b. There are many such instances in unigine heaven workload (there would
>>>> be many such workloads), throwing a normal error would lower the FPS? is
>>>> it possible to fix at caller application?
>>>>
>>>> c. adding the round_up() is possible, but in every such instances we end
>>>> up allocating extra unused memory. For example, if required pages = 1028
>>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>>> to the min_page_size, so the total size would be 2048 pages.
>>>>
>>>>> i.e if someone does:
>>>>>
>>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>>> CASE 2:
>>>> I think this case should be detected (i.e) when min_page_size > size,
>>>> should we return -EINVAL?
>>>>>
>>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>>> AFAICT.
>>>>>
>>>>
>>>> Should we just allow the CASE 1 proceed for the allocation and return
>>>> -EINVAL for the CASE 2?
>>>>
>>>>>> +
>>>>>>     	do {
>>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>>     		BUG_ON(order > mm->max_order);
>>>>>> -		BUG_ON(order < min_order);
>>>>>>     
>>>>>>     		do {
>>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>>
>>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b

^ permalink raw reply	[flat|nested] 32+ messages in thread
end of thread, other threads:[~2022-03-14 19:27 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 14:37 [PATCH] drm: remove min_order BUG_ON check Arunpravin
2022-03-07 14:37 ` Arunpravin
2022-03-07 14:37 ` [Intel-gfx] " Arunpravin
2022-03-07 14:45 ` Jani Nikula
2022-03-08 14:32   ` Arunpravin
2022-03-07 15:04 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2022-03-07 15:36 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-03-07 15:53 ` [PATCH] " Christian König
2022-03-07 15:53   ` Christian König
2022-03-07 15:53   ` [Intel-gfx] " Christian König
2022-03-08 14:27   ` Arunpravin
2022-03-08 14:27     ` Arunpravin
2022-03-08 14:27     ` [Intel-gfx] " Arunpravin
2022-03-07 16:41 ` Matthew Auld
2022-03-07 16:41   ` Matthew Auld
2022-03-07 16:41   ` [Intel-gfx] " Matthew Auld
2022-03-08 13:59   ` Arunpravin
2022-03-08 13:59     ` Arunpravin
2022-03-08 13:59     ` [Intel-gfx] " Arunpravin
2022-03-08 17:01     ` Matthew Auld
2022-03-08 17:01       ` Matthew Auld
2022-03-08 17:01       ` [Intel-gfx] " Matthew Auld
2022-03-10 14:47       ` Arunpravin
2022-03-10 14:47         ` Arunpravin
2022-03-10 14:47         ` [Intel-gfx] " Arunpravin
2022-03-10 15:29         ` Matthew Auld
2022-03-10 15:29           ` Matthew Auld
2022-03-10 15:29           ` [Intel-gfx] " Matthew Auld
2022-03-14 19:38           ` Arunpravin
2022-03-14 19:38             ` Arunpravin
2022-03-14 19:38             ` [Intel-gfx] " Arunpravin
2022-03-07 18:31 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.