Re: [Intel-gfx] [PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Iddamsetty, Aravind 



On 03-02-2023 17:40, Tvrtko Ursulin wrote:
> 
> 
> On 03/02/2023 11:57, Aravind Iddamsetty wrote:
>> Obj flags for shmem objects is not being set correctly.
>>
>> Cc: Matthew Auld 
>> Signed-off-by: Aravind Iddamsetty 
> 
> Could even be:
> 
> Fixes: 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on
> acquire")
> Cc:  # v5.15+
> 
> ?
Thanks for the review, will resend with this.

Aravind.
> 
> Regards,
> 
> Tvrtko
> 
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 114443096841..37d1efcd3ca6 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -596,7 +596,7 @@ static int shmem_object_init(struct
>> intel_memory_region *mem,
>>   mapping_set_gfp_mask(mapping, mask);
>>   GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>>   -    i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);
>> +    i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
>>   obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>   obj->write_domain = I915_GEM_DOMAIN_CPU;
>>   obj->read_domains = I915_GEM_DOMAIN_CPU;

Re: [PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Iddamsetty, Aravind 



On 03-02-2023 17:42, Matthew Auld wrote:
> On 03/02/2023 11:57, Aravind Iddamsetty wrote:
>> Obj flags for shmem objects is not being set correctly.
>>
>> Cc: Matthew Auld 
>> Signed-off-by: Aravind Iddamsetty 
> 
> Subject should have "drm/i915:" prefix.
My bad missed that.
> 
> This is also a bug fix due to not setting BO_ALLOC_USER (the other flags
> don't seem to matter for shmem), which is quite important, so we need to
> figure out the "Fixes" tag. Maybe mention in the commit message that
> this fixes setting ALLOC_USER which is needed even for shmem.
> 
> Looking at the git history, ALLOC_USER looks to be first introduced in
> 213d50927763 ("drm/i915/ttm: Introduce a TTM i915 gem object backend"),
> but the users of ALLOC_USER at this stage are only interesting for the
> ttm backend, and that already passes the flags due to using its own
> object_init() vfunc for all normal object types.
> 
> So the first real user impacted by this bug appears to be in:
> 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on acquire").
> 
> So I think needs:
> 
> Fixes: 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on
> acquire")
> Cc:  # v5.15+
> 
> With that,
> Reviewed-by: Matthew Auld 
Thank you will resend the patch.

Regards,
Aravind.
> 
> 
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 114443096841..37d1efcd3ca6 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -596,7 +596,7 @@ static int shmem_object_init(struct
>> intel_memory_region *mem,
>>   mapping_set_gfp_mask(mapping, mask);
>>   GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>>   -    i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);
>> +    i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
>>   obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>   obj->write_domain = I915_GEM_DOMAIN_CPU;
>>   obj->read_domains = I915_GEM_DOMAIN_CPU;

Re: [Intel-gfx] [PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Matthew Auld 

On 03/02/2023 12:10, Tvrtko Ursulin wrote:



On 03/02/2023 11:57, Aravind Iddamsetty wrote:

Obj flags for shmem objects is not being set correctly.

Cc: Matthew Auld 
Signed-off-by: Aravind Iddamsetty 


Could even be:

Fixes: 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on 
acquire")

Cc:  # v5.15+


Yup, that's what I also got.



?

Regards,

Tvrtko


---
  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c

index 114443096841..37d1efcd3ca6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -596,7 +596,7 @@ static int shmem_object_init(struct 
intel_memory_region *mem,

  mapping_set_gfp_mask(mapping, mask);
  GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
-    i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);
+    i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
  obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
  obj->write_domain = I915_GEM_DOMAIN_CPU;
  obj->read_domains = I915_GEM_DOMAIN_CPU;

Re: [PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Matthew Auld 

On 03/02/2023 11:57, Aravind Iddamsetty wrote:

Obj flags for shmem objects is not being set correctly.

Cc: Matthew Auld 
Signed-off-by: Aravind Iddamsetty 


Subject should have "drm/i915:" prefix.

This is also a bug fix due to not setting BO_ALLOC_USER (the other flags 
don't seem to matter for shmem), which is quite important, so we need to 
figure out the "Fixes" tag. Maybe mention in the commit message that 
this fixes setting ALLOC_USER which is needed even for shmem.


Looking at the git history, ALLOC_USER looks to be first introduced in 
213d50927763 ("drm/i915/ttm: Introduce a TTM i915 gem object backend"), 
but the users of ALLOC_USER at this stage are only interesting for the 
ttm backend, and that already passes the flags due to using its own 
object_init() vfunc for all normal object types.


So the first real user impacted by this bug appears to be in: 
13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on acquire").


So I think needs:

Fixes: 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on 
acquire")

Cc:  # v5.15+

With that,
Reviewed-by: Matthew Auld 



---
  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 114443096841..37d1efcd3ca6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -596,7 +596,7 @@ static int shmem_object_init(struct intel_memory_region 
*mem,
mapping_set_gfp_mask(mapping, mask);
GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
  
-	i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);

+   i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->write_domain = I915_GEM_DOMAIN_CPU;
obj->read_domains = I915_GEM_DOMAIN_CPU;

Re: [Intel-gfx] [PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Tvrtko Ursulin 




On 03/02/2023 11:57, Aravind Iddamsetty wrote:

Obj flags for shmem objects is not being set correctly.

Cc: Matthew Auld 
Signed-off-by: Aravind Iddamsetty 


Could even be:

Fixes: 13d29c823738 ("drm/i915/ehl: unconditionally flush the pages on acquire")
Cc:  # v5.15+

?

Regards,

Tvrtko


---
  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 114443096841..37d1efcd3ca6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -596,7 +596,7 @@ static int shmem_object_init(struct intel_memory_region 
*mem,
mapping_set_gfp_mask(mapping, mask);
GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
  
-	i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);

+   i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->write_domain = I915_GEM_DOMAIN_CPU;
obj->read_domains = I915_GEM_DOMAIN_CPU;

[PATCH] Initialize the obj flags for shmem objects

2023-02-03 Thread Aravind Iddamsetty 
Obj flags for shmem objects is not being set correctly.

Cc: Matthew Auld 
Signed-off-by: Aravind Iddamsetty 
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 114443096841..37d1efcd3ca6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -596,7 +596,7 @@ static int shmem_object_init(struct intel_memory_region 
*mem,
mapping_set_gfp_mask(mapping, mask);
GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
 
-   i915_gem_object_init(obj, _gem_shmem_ops, _class, 0);
+   i915_gem_object_init(obj, _gem_shmem_ops, _class, flags);
obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->write_domain = I915_GEM_DOMAIN_CPU;
obj->read_domains = I915_GEM_DOMAIN_CPU;
-- 
2.25.1