diff mbox series

[v2,3/5] drm/xe/display: Avoid calling readq()

Message ID 20240116174050.2435923-4-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Fix 32bit build | expand

Commit Message

Lucas De Marchi Jan. 16, 2024, 5:40 p.m. UTC
readq() is not available in 32bits. iosys-map already has the logic in
place to use read u64 in all cases, so simply add a helper variable for
using that.

Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 .../gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h   | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Matt Roper Jan. 18, 2024, 12:15 a.m. UTC | #1
On Tue, Jan 16, 2024 at 09:40:48AM -0800, Lucas De Marchi wrote:
> readq() is not available in 32bits. iosys-map already has the logic in
> place to use read u64 in all cases, so simply add a helper variable for
> using that.
> 
> Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  .../gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h   | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
> index 5f19550cc845..6739dadaf1a9 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
> @@ -7,6 +7,7 @@
>  #define _I915_GEM_OBJECT_H_
>  
>  #include <linux/types.h>
> +#include <linux/iosys-map.h>
>  
>  #include "xe_bo.h"
>  
> @@ -36,6 +37,7 @@ static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
>  {
>  	struct ttm_bo_kmap_obj map;
>  	void *virtual;
> +	struct iosys_map vaddr;
>  	bool is_iomem;
>  	int ret;
>  
> @@ -52,10 +54,11 @@ static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
>  	ofs &= ~PAGE_MASK;
>  	virtual = ttm_kmap_obj_virtual(&map, &is_iomem);
>  	if (is_iomem)
> -		*ptr = readq((void __iomem *)(virtual + ofs));
> +		iosys_map_set_vaddr_iomem(&vaddr, (void __iomem *)(virtual));

Should we just use a memcpy_fromio (and memcpy in the else branch) and
pass the size actually requested rather than hardcoding it to a u64?  At
the moment the only callsite happens to want a u64, and thus the Xe
compat header has an XE_WARN_ON that complains if any other size is
requested, but in theory this function is supposed to be general purpose
and take any size.


Matt

>  	else
> -		*ptr = *(u64 *)(virtual + ofs);
> +		iosys_map_set_vaddr(&vaddr, virtual);
>  
> +	*ptr = iosys_map_rd(&vaddr, ofs, u64);
>  	ttm_bo_kunmap(&map);
>  out_unlock:
>  	xe_bo_unlock(bo);
> -- 
> 2.40.1
>
Lucas De Marchi Jan. 18, 2024, 10:01 p.m. UTC | #2
On Wed, Jan 17, 2024 at 04:15:42PM -0800, Matt Roper wrote:
>On Tue, Jan 16, 2024 at 09:40:48AM -0800, Lucas De Marchi wrote:
>> readq() is not available in 32bits. iosys-map already has the logic in
>> place to use read u64 in all cases, so simply add a helper variable for
>> using that.
>>
>> Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  .../gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h   | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> index 5f19550cc845..6739dadaf1a9 100644
>> --- a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> @@ -7,6 +7,7 @@
>>  #define _I915_GEM_OBJECT_H_
>>
>>  #include <linux/types.h>
>> +#include <linux/iosys-map.h>
>>
>>  #include "xe_bo.h"
>>
>> @@ -36,6 +37,7 @@ static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
>>  {
>>  	struct ttm_bo_kmap_obj map;
>>  	void *virtual;
>> +	struct iosys_map vaddr;
>>  	bool is_iomem;
>>  	int ret;
>>
>> @@ -52,10 +54,11 @@ static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
>>  	ofs &= ~PAGE_MASK;
>>  	virtual = ttm_kmap_obj_virtual(&map, &is_iomem);
>>  	if (is_iomem)
>> -		*ptr = readq((void __iomem *)(virtual + ofs));
>> +		iosys_map_set_vaddr_iomem(&vaddr, (void __iomem *)(virtual));
>
>Should we just use a memcpy_fromio (and memcpy in the else branch) and
>pass the size actually requested rather than hardcoding it to a u64?  At
>the moment the only callsite happens to want a u64, and thus the Xe
>compat header has an XE_WARN_ON that complains if any other size is
>requested, but in theory this function is supposed to be general purpose
>and take any size.

yeah, good catch. I will update this on next version.

thanks
Lucas De Marchi

>
>
>Matt
>
>>  	else
>> -		*ptr = *(u64 *)(virtual + ofs);
>> +		iosys_map_set_vaddr(&vaddr, virtual);
>>
>> +	*ptr = iosys_map_rd(&vaddr, ofs, u64);
>>  	ttm_bo_kunmap(&map);
>>  out_unlock:
>>  	xe_bo_unlock(bo);
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
index 5f19550cc845..6739dadaf1a9 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
@@ -7,6 +7,7 @@ 
 #define _I915_GEM_OBJECT_H_
 
 #include <linux/types.h>
+#include <linux/iosys-map.h>
 
 #include "xe_bo.h"
 
@@ -36,6 +37,7 @@  static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
 {
 	struct ttm_bo_kmap_obj map;
 	void *virtual;
+	struct iosys_map vaddr;
 	bool is_iomem;
 	int ret;
 
@@ -52,10 +54,11 @@  static inline int i915_gem_object_read_from_page(struct xe_bo *bo,
 	ofs &= ~PAGE_MASK;
 	virtual = ttm_kmap_obj_virtual(&map, &is_iomem);
 	if (is_iomem)
-		*ptr = readq((void __iomem *)(virtual + ofs));
+		iosys_map_set_vaddr_iomem(&vaddr, (void __iomem *)(virtual));
 	else
-		*ptr = *(u64 *)(virtual + ofs);
+		iosys_map_set_vaddr(&vaddr, virtual);
 
+	*ptr = iosys_map_rd(&vaddr, ofs, u64);
 	ttm_bo_kunmap(&map);
 out_unlock:
 	xe_bo_unlock(bo);