[PATCH v2 0/3] drm/i915: i915_gem_execbuffer.c minor improved

[PATCH v2 0/3] drm/i915: i915_gem_execbuffer.c minor improved

[Sebastian Brzezinka]

While debugging, I identified and addressed a few minor issues and
style inconsistencies that could be improved.

I ran some tests on these changes, and pushed them to trybot
first, no regressions were found.

Sebastian Brzezinka (3):
  drm/i915: Improve readability of eb_relocate_vma
  drm/i915: Add braces around the else block in clflush_write32()
  drm/i915: Replaced hardcoded value 4095 with PAGE_SIZE - 1

 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 90 ++++++++++---------
 1 file changed, 47 insertions(+), 43 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] drm/i915: Improve readability of eb_relocate_vma

[Sebastian Brzezinka]

Refactored eb_relocate_vma to simplify stack access and loop structure.
No functional changes; this is a readability and clarity improvement only.

Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
v1 -> v2:
	- Revert changes in error handling.
	- Restore the original formatting of the comments.
	- Reword commit message.
---
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 84 ++++++++++---------
 1 file changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index ca7e9216934a..ea4793e73ea5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1529,6 +1529,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
 	struct drm_i915_gem_relocation_entry __user *urelocs =
 		u64_to_user_ptr(entry->relocs_ptr);
 	unsigned long remain = entry->relocation_count;
+	int ret = 0;
 
 	if (unlikely(remain > N_RELOC(INT_MAX)))
 		return -EINVAL;
@@ -1541,11 +1542,10 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
 	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
 		return -EFAULT;
 
-	do {
-		struct drm_i915_gem_relocation_entry *r = stack;
-		unsigned int count =
-			min_t(unsigned long, remain, ARRAY_SIZE(stack));
+	while (remain > 0) {
+		unsigned int count = min_t(unsigned long, remain, ARRAY_SIZE(stack));
 		unsigned int copied;
+		unsigned int i;
 
 		/*
 		 * This is the fast path and we cannot handle a pagefault
@@ -1556,53 +1556,57 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
 		 * this is bad and so lockdep complains vehemently.
 		 */
 		pagefault_disable();
-		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
+		copied = __copy_from_user_inatomic(stack, urelocs, count * sizeof(stack[0]));
 		pagefault_enable();
+
 		if (unlikely(copied)) {
-			remain = -EFAULT;
+			ret = -EFAULT;
 			goto out;
 		}
 
-		remain -= count;
-		do {
+		for (i = 0; i < count; ++i) {
+			struct drm_i915_gem_relocation_entry *r = &stack[i];
 			u64 offset = eb_relocate_entry(eb, ev, r);
 
-			if (likely(offset == 0)) {
-			} else if ((s64)offset < 0) {
-				remain = (int)offset;
+			if (likely(offset == 0))
+				continue;
+
+			if (unlikely((s64)offset < 0)) {
+				ret = (int)offset;
 				goto out;
-			} else {
-				/*
-				 * Note that reporting an error now
-				 * leaves everything in an inconsistent
-				 * state as we have *already* changed
-				 * the relocation value inside the
-				 * object. As we have not changed the
-				 * reloc.presumed_offset or will not
-				 * change the execobject.offset, on the
-				 * call we may not rewrite the value
-				 * inside the object, leaving it
-				 * dangling and causing a GPU hang. Unless
-				 * userspace dynamically rebuilds the
-				 * relocations on each execbuf rather than
-				 * presume a static tree.
-				 *
-				 * We did previously check if the relocations
-				 * were writable (access_ok), an error now
-				 * would be a strange race with mprotect,
-				 * having already demonstrated that we
-				 * can read from this userspace address.
-				 */
-				offset = gen8_canonical_addr(offset & ~UPDATE);
-				__put_user(offset,
-					   &urelocs[r - stack].presumed_offset);
 			}
-		} while (r++, --count);
-		urelocs += ARRAY_SIZE(stack);
-	} while (remain);
+			/*
+			 * Note that reporting an error now
+			 * leaves everything in an inconsistent
+			 * state as we have *already* changed
+			 * the relocation value inside the
+			 * object. As we have not changed the
+			 * reloc.presumed_offset or will not
+			 * change the execobject.offset, on the
+			 * call we may not rewrite the value
+			 * inside the object, leaving it
+			 * dangling and causing a GPU hang. Unless
+			 * userspace dynamically rebuilds the
+			 * relocations on each execbuf rather than
+			 * presume a static tree.
+			 *
+			 * We did previously check if the relocations
+			 * were writable (access_ok), an error now
+			 * would be a strange race with mprotect,
+			 * having already demonstrated that we
+			 * can read from this userspace address.
+			 */
+			offset = gen8_canonical_addr(offset &  ~UPDATE);
+			__put_user(offset, &urelocs[i].presumed_offset);
+		}
+
+		remain -= count;
+		urelocs += count;
+	}
+
 out:
 	reloc_cache_reset(&eb->reloc_cache, eb);
-	return remain;
+	return ret;
 }
 
 static int
-- 
2.34.1

[PATCH v2 2/3] drm/i915: Add braces around the else block in clflush_write32()

[Sebastian Brzezinka]

According to the kernel coding style, if only one branch of a
conditional statement is a single statement, braces should
still be used in both branches.

Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
v1 -> v2:
	- Add commit body.
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index ea4793e73ea5..e3ddceb7bd97 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1382,8 +1382,9 @@ static void clflush_write32(u32 *addr, u32 value, unsigned int flushes)
 		 */
 		if (flushes & CLFLUSH_AFTER)
 			drm_clflush_virt_range(addr, sizeof(*addr));
-	} else
+	} else {
 		*addr = value;
+	}
 }
 
 static u64
-- 
2.34.1

[PATCH v2 3/3] drm/i915: Replaced hardcoded value 4095 with PAGE_SIZE - 1

[Sebastian Brzezinka]

The value 4095 is most likely intended to represent PAGE_SIZE - 1.

Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
v1 -> v2:
	- Resend, no changes.
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e3ddceb7bd97..64d349ca7a89 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -397,7 +397,7 @@ eb_vma_misplaced(const struct drm_i915_gem_exec_object2 *entry,
 		return true;
 
 	if (!(flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS) &&
-	    (start + size + 4095) >> 32)
+	    (start + size + PAGE_SIZE - 1) >> 32)
 		return true;
 
 	if (flags & __EXEC_OBJECT_NEEDS_MAP &&
-- 
2.34.1

✗ LGCI.VerificationFailed: failure for drm/i915: i915_gem_execbuffer.c minor improved (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: i915_gem_execbuffer.c minor improved (rev2)
URL   : https://patchwork.freedesktop.org/series/151697/
State : failure

== Summary ==

Exception occurred during validation, bailing out!

Re: [PATCH v2 1/3] drm/i915: Improve readability of eb_relocate_vma

[Krzysztof Karas]

Hi Sebastian,

> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1529,6 +1529,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  	struct drm_i915_gem_relocation_entry __user *urelocs =
>  		u64_to_user_ptr(entry->relocs_ptr);
>  	unsigned long remain = entry->relocation_count;
> +	int ret = 0;
>  
>  	if (unlikely(remain > N_RELOC(INT_MAX)))
>  		return -EINVAL;
> @@ -1541,11 +1542,10 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
>  		return -EFAULT;
>  
> -	do {
> -		struct drm_i915_gem_relocation_entry *r = stack;
> -		unsigned int count =
> -			min_t(unsigned long, remain, ARRAY_SIZE(stack));
> +	while (remain > 0) {
Is it possible for "remain" variable to be initialized to 0?
If that would be the case then after your change we'd skip this
loop entirely, where previously we'd run at least one iteration.
Would that be a problem?

Apart from that, I like that you removed the empty "if" and
reduced indentation :)

-- 
Best Regards,
Krzysztof

Re: [PATCH v2 2/3] drm/i915: Add braces around the else block in clflush_write32()

[Krzysztof Karas]

Hi Sebastian,

> According to the kernel coding style, if only one branch of a
> conditional statement is a single statement, braces should
> still be used in both branches.
> 
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
> v1 -> v2:
> 	- Add commit body.
> ---

Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>

-- 
Best Regards,
Krzysztof

Re: [PATCH v2 3/3] drm/i915: Replaced hardcoded value 4095 with PAGE_SIZE - 1

[Krzysztof Karas]

Hi Sebastian,

> The value 4095 is most likely intended to represent PAGE_SIZE - 1.
Andi noticed in the previous version of this patch that
"most likely" might be too vague and I agree. The original
commit that introduced this magic number 4095:
5f22cc0b134ab702d7f64b714e26018f7288ffee, which explains a bit
why it was introduced:

    "commit 48ea1e32c39d ("drm/i915/gen9: Set PIN_ZONE_4G end to 4GB - 1
    page") suggests that there is a genuine problem with stateless addressing
    that cannot utilize the last page in 4G and so we purposefully excluded
    it. This means that the quick pin pass may cause us to utilize a buggy
    placement."

So in this case I'd assume that change from " - 1" to "+ 4095"
is indeed supposed to include that last page (which is of
PAGE_SIZE length).
Would you mind checking out that commit and seeing if you agree
with my assumptions? If yes, then please add a reference to the
commit 5f22cc0b134ab702d7f64b714e26018f7288ffee and change
"most likely" part in the commit message.

-- 
Best Regards,
Krzysztof

Re: [PATCH v2 3/3] drm/i915: Replaced hardcoded value 4095 with PAGE_SIZE - 1

[Andi Shyti]

Hi Sebastian,

let's start over...

On Thu, Jul 17, 2025 at 12:38:56PM +0000, Sebastian Brzezinka wrote:
> The value 4095 is most likely intended to represent PAGE_SIZE - 1.

most likely? We need to be sure :-)

Andi

Re: [PATCH v2 1/3] drm/i915: Improve readability of eb_relocate_vma

[Andi Shyti]

Hi Sebastian,

On Thu, Jul 17, 2025 at 12:38:39PM +0000, Sebastian Brzezinka wrote:
> Refactored eb_relocate_vma to simplify stack access and loop structure.
> No functional changes; this is a readability and clarity improvement only.

There is no limit for a commit log. You need to list all the
changes you did, a generic "refactor" is not enough.

Bonus: please use the imperative form, "Refactor", not
"Refactored". Imperative is always clearer.

> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
> v1 -> v2:
> 	- Revert changes in error handling.
> 	- Restore the original formatting of the comments.
> 	- Reword commit message.
> ---
>  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 84 ++++++++++---------
>  1 file changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index ca7e9216934a..ea4793e73ea5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1529,6 +1529,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  	struct drm_i915_gem_relocation_entry __user *urelocs =
>  		u64_to_user_ptr(entry->relocs_ptr);
>  	unsigned long remain = entry->relocation_count;
> +	int ret = 0;
>  
>  	if (unlikely(remain > N_RELOC(INT_MAX)))
>  		return -EINVAL;
> @@ -1541,11 +1542,10 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
>  		return -EFAULT;
>  
> -	do {
> -		struct drm_i915_gem_relocation_entry *r = stack;
> -		unsigned int count =
> -			min_t(unsigned long, remain, ARRAY_SIZE(stack));
> +	while (remain > 0) {

what's the problem with the "do ... while()"?

> +		unsigned int count = min_t(unsigned long, remain, ARRAY_SIZE(stack));
>  		unsigned int copied;
> +		unsigned int i;
>  
>  		/*
>  		 * This is the fast path and we cannot handle a pagefault
> @@ -1556,53 +1556,57 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>  		 * this is bad and so lockdep complains vehemently.
>  		 */
>  		pagefault_disable();
> -		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> +		copied = __copy_from_user_inatomic(stack, urelocs, count * sizeof(stack[0]));

What is the improvement here?

>  		pagefault_enable();
> +
>  		if (unlikely(copied)) {
> -			remain = -EFAULT;
> +			ret = -EFAULT;
>  			goto out;
>  		}
>  
> -		remain -= count;
> -		do {
> +		for (i = 0; i < count; ++i) {

what's wrong with the "do ... while()".

Generally speaking, personal preference is not a valid reason for
sending a patch. Changing a clear do/while with a clear while or
for does not improve anything.

> +			struct drm_i915_gem_relocation_entry *r = &stack[i];
>  			u64 offset = eb_relocate_entry(eb, ev, r);
>  
> -			if (likely(offset == 0)) {
> -			} else if ((s64)offset < 0) {
> -				remain = (int)offset;
> +			if (likely(offset == 0))
> +				continue;
> +
> +			if (unlikely((s64)offset < 0)) {
> +				ret = (int)offset;

This change here is at the limit between readability and personal
preference. Still acceptable, though because it looks nicer.

BTW, the cast is not necessary.

>  				goto out;
> -			} else {
> -				/*
> -				 * Note that reporting an error now
> -				 * leaves everything in an inconsistent
> -				 * state as we have *already* changed
> -				 * the relocation value inside the
> -				 * object. As we have not changed the
> -				 * reloc.presumed_offset or will not
> -				 * change the execobject.offset, on the
> -				 * call we may not rewrite the value
> -				 * inside the object, leaving it
> -				 * dangling and causing a GPU hang. Unless
> -				 * userspace dynamically rebuilds the
> -				 * relocations on each execbuf rather than
> -				 * presume a static tree.
> -				 *
> -				 * We did previously check if the relocations
> -				 * were writable (access_ok), an error now
> -				 * would be a strange race with mprotect,
> -				 * having already demonstrated that we
> -				 * can read from this userspace address.
> -				 */
> -				offset = gen8_canonical_addr(offset & ~UPDATE);
> -				__put_user(offset,
> -					   &urelocs[r - stack].presumed_offset);
>  			}
> -		} while (r++, --count);
> -		urelocs += ARRAY_SIZE(stack);
> -	} while (remain);
> +			/*
> +			 * Note that reporting an error now
> +			 * leaves everything in an inconsistent
> +			 * state as we have *already* changed
> +			 * the relocation value inside the
> +			 * object. As we have not changed the
> +			 * reloc.presumed_offset or will not
> +			 * change the execobject.offset, on the
> +			 * call we may not rewrite the value
> +			 * inside the object, leaving it
> +			 * dangling and causing a GPU hang. Unless
> +			 * userspace dynamically rebuilds the
> +			 * relocations on each execbuf rather than
> +			 * presume a static tree.
> +			 *
> +			 * We did previously check if the relocations
> +			 * were writable (access_ok), an error now
> +			 * would be a strange race with mprotect,
> +			 * having already demonstrated that we
> +			 * can read from this userspace address.
> +			 */
> +			offset = gen8_canonical_addr(offset &  ~UPDATE);
> +			__put_user(offset, &urelocs[i].presumed_offset);
> +		}
> +
> +		remain -= count;
> +		urelocs += count;
> +	}
> +
>  out:
>  	reloc_cache_reset(&eb->reloc_cache, eb);
> -	return remain;
> +	return ret;

Using ret instead of remain is not a bad change, still at the
limit of the personal preference. It's OK because:

1. it avoids mistake from code glancers like myself in v1.
2. it avoids some useless casts and alligns the types.

So, overall, I see only two parts that can stay in this patch:

1. the if/else re-arrangement you did above
2. the use of ret.

They should be sent in two different patches.

The rest looks to me just personal preference and it wouldn't
make sense to push it through.

Andi

>  }
>  
>  static int
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/3] drm/i915: Add braces around the else block in clflush_write32()

[Andi Shyti]

Hi Sebastian,

On Thu, Jul 17, 2025 at 12:38:47PM +0000, Sebastian Brzezinka wrote:
> According to the kernel coding style, if only one branch of a
> conditional statement is a single statement, braces should
> still be used in both branches.
> 
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH v2 1/3] drm/i915: Improve readability of eb_relocate_vma

[Sebastian Brzezinka]

Hi Krzysztof
On Thu Jul 17, 2025 at 2:38 PM UTC, Krzysztof Karas wrote:
> Hi Sebastian,
>
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -1529,6 +1529,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>>  	struct drm_i915_gem_relocation_entry __user *urelocs =
>>  		u64_to_user_ptr(entry->relocs_ptr);
>>  	unsigned long remain = entry->relocation_count;
>> +	int ret = 0;
>>  
>>  	if (unlikely(remain > N_RELOC(INT_MAX)))
>>  		return -EINVAL;
>> @@ -1541,11 +1542,10 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
>>  	if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
>>  		return -EFAULT;
>>  
>> -	do {
>> -		struct drm_i915_gem_relocation_entry *r = stack;
>> -		unsigned int count =
>> -			min_t(unsigned long, remain, ARRAY_SIZE(stack));
>> +	while (remain > 0) {
> Is it possible for "remain" variable to be initialized to 0?
> If that would be the case then after your change we'd skip this
> loop entirely, where previously we'd run at least one iteration.
> Would that be a problem?
-- 
This should not occur, as there are several direct and indirect safeguards
verifying that entry->relocation_count is non-zero.
e.g.
```eb_relocate_vma_slow
1622 ›       for (i = 0; i < entry->relocation_count; i++) {
```
or

```check_relocations
1642 ›       size = entry->relocation_count;
1643 ›       if (size == 0)
1644 ›       ›       return 0;
```
And it would be a peculiar choice to copy 0 bytes

Best regards,
Sebastian