Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

2023-12-13 Thread Ville Syrjälä 
On Wed, Dec 13, 2023 at 02:59:07AM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 02:42:26AM +0200, Ville Syrjala wrote:
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c 
> > b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > index 717c3a3237c4..1ac05d90b2e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private 
> > *i915, struct fb_info *info
> >  
> > /* Use fbdev's framebuffer from lmem for discrete */
> > info->fix.smem_start =
> > -   (unsigned long)(mem->io_start +
> > +   (unsigned long)(mem->io.start +
> > i915_gem_object_get_dma_address(obj, 
> > 0));
> 
> Hmm. That looks wrong for MTL actually since dma address is relative
> to the start of LMEMBAR but stolen io.start will be LMEMBAR+8MiB (or
> just DSMBASE which points to the same place, with the w/a in place).
> So we need to subtract mem->region.start from this to get the correct
> value.

I suppose this doesn't matter anymore as we have our own .fb_mmap()
these days. So presumably we could just always leave this set to zero.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

2023-12-13 Thread Ville Syrjälä 
On Wed, Dec 13, 2023 at 04:52:54PM +0100, Andrzej Hajda wrote:
> On 13.12.2023 01:42, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > mem->region is a struct resource, but mem->io_start and
> > mem->io_size are not for whatever reason. Let's unify this
> > and convert the io stuff into a struct resource as well.
> > Should make life a little less annoying when you don't have
> > juggle between two different approaches all the time.
> > 
> > Mostly done using cocci (with manual tweaks at all the
> > places where we mutate io_size by hand):
> > @@
> > struct intel_memory_region *M;
> > expression START, SIZE;
> > @@
> > - M->io_start = START;
> > - M->io_size = SIZE;
> > + M->io = DEFINE_RES_MEM(START, SIZE);
> > 
> > @@
> > struct intel_memory_region *M;
> > @@
> > - M->io_start
> > + M->io.start
> > 
> > @@
> > struct intel_memory_region M;
> > @@
> > - M.io_start
> > + M.io.start
> > 
> > @@
> > expression M;
> > @@
> > - M->io_size
> > + resource_size(&M->io)
> > 
> > @@
> > expression M;
> > @@
> > - M.io_size
> > + resource_size(&M.io)
> > 
> > Signed-off-by: Ville Syrjälä 
> 
> 
> You can try to replace local vars and/or arguments as well:
> In i915_gem_stolen_lmem_setup:
>   resource_size_t io_start, io_size;
> 
> (intel_memory|mock)_region_create(struct drm_i915_private *i915,
>  resource_size_t start,
>  resource_size_t size,
>  resource_size_t min_page_size,
>  resource_size_t io_start,
>  resource_size_t io_size,
>  u16 type,
>  u16 instance,
>  const struct intel_memory_region_ops *ops);

I think it's easier to let that guy deal with the
size->end conversion. The callers are generally
more interested in the size itself.

> 
> With or without this change:
> Reviewed-by: Andrzej Hajda 

Thanks

> 
> Regards
> Andrzej
> 
> 
> > ---
> >   drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_region.c |  2 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 17 +
> >   drivers/gpu/drm/i915/gem/i915_gem_ttm.c|  8 
> >   .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +-
> >   drivers/gpu/drm/i915/gt/intel_region_lmem.c| 11 +++
> >   drivers/gpu/drm/i915/gt/selftest_tlb.c |  4 ++--
> >   drivers/gpu/drm/i915/i915_gpu_error.c  |  2 +-
> >   drivers/gpu/drm/i915/i915_query.c  |  2 +-
> >   drivers/gpu/drm/i915/intel_memory_region.c | 15 +++
> >   drivers/gpu/drm/i915/intel_memory_region.h |  3 +--
> >   drivers/gpu/drm/i915/intel_region_ttm.c|  8 
> >   .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
> >   13 files changed, 45 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c 
> > b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > index 717c3a3237c4..1ac05d90b2e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private 
> > *i915, struct fb_info *info
> >   
> > /* Use fbdev's framebuffer from lmem for discrete */
> > info->fix.smem_start =
> > -   (unsigned long)(mem->io_start +
> > +   (unsigned long)(mem->io.start +
> > i915_gem_object_get_dma_address(obj, 
> > 0));
> > info->fix.smem_len = obj->base.size;
> > } else {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > index a4fb577eceb4..b09b74a2448b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > @@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct 
> > intel_memory_region *mem,
> > return ERR_PTR(-EINVAL);
> >   
> > if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&
> > -   offset + size > mem->io_size &&
> > +   offset + size > resource_size(&mem->io) &&
> > !i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
> > return ERR_PTR(-ENOSPC);
> >   
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > index 8c88075eeab2..d2440c793f84 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > @@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct 
> > intel_memory_region *mem)
> >   
> > /* Exclude the reserved region from driver use */
> > mem->region.end = i915->dsm.reserved.start - 1;
> > -   mem->io_size = min(mem->io_size, resource_size(&mem->region));
> > +   mem->io = DEFINE_RES_MEM(mem->io.start,
> > +min(resource_size(&mem->io),
> > +

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

2023-12-13 Thread Andrzej Hajda 

On 13.12.2023 01:42, Ville Syrjala wrote:

From: Ville Syrjälä 

mem->region is a struct resource, but mem->io_start and
mem->io_size are not for whatever reason. Let's unify this
and convert the io stuff into a struct resource as well.
Should make life a little less annoying when you don't have
juggle between two different approaches all the time.

Mostly done using cocci (with manual tweaks at all the
places where we mutate io_size by hand):
@@
struct intel_memory_region *M;
expression START, SIZE;
@@
- M->io_start = START;
- M->io_size = SIZE;
+ M->io = DEFINE_RES_MEM(START, SIZE);

@@
struct intel_memory_region *M;
@@
- M->io_start
+ M->io.start

@@
struct intel_memory_region M;
@@
- M.io_start
+ M.io.start

@@
expression M;
@@
- M->io_size
+ resource_size(&M->io)

@@
expression M;
@@
- M.io_size
+ resource_size(&M.io)

Signed-off-by: Ville Syrjälä 



You can try to replace local vars and/or arguments as well:
In i915_gem_stolen_lmem_setup:
resource_size_t io_start, io_size;

(intel_memory|mock)_region_create(struct drm_i915_private *i915,
   resource_size_t start,
   resource_size_t size,
   resource_size_t min_page_size,
   resource_size_t io_start,
   resource_size_t io_size,
   u16 type,
   u16 instance,
   const struct intel_memory_region_ops *ops);

With or without this change:
Reviewed-by: Andrzej Hajda 

Regards
Andrzej



---
  drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
  drivers/gpu/drm/i915/gem/i915_gem_region.c |  2 +-
  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 17 +
  drivers/gpu/drm/i915/gem/i915_gem_ttm.c|  8 
  .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +-
  drivers/gpu/drm/i915/gt/intel_region_lmem.c| 11 +++
  drivers/gpu/drm/i915/gt/selftest_tlb.c |  4 ++--
  drivers/gpu/drm/i915/i915_gpu_error.c  |  2 +-
  drivers/gpu/drm/i915/i915_query.c  |  2 +-
  drivers/gpu/drm/i915/intel_memory_region.c | 15 +++
  drivers/gpu/drm/i915/intel_memory_region.h |  3 +--
  drivers/gpu/drm/i915/intel_region_ttm.c|  8 
  .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
  13 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c 
b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
index 717c3a3237c4..1ac05d90b2e8 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
@@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, 
struct fb_info *info
  
  		/* Use fbdev's framebuffer from lmem for discrete */

info->fix.smem_start =
-   (unsigned long)(mem->io_start +
+   (unsigned long)(mem->io.start +
i915_gem_object_get_dma_address(obj, 
0));
info->fix.smem_len = obj->base.size;
} else {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index a4fb577eceb4..b09b74a2448b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct intel_memory_region 
*mem,
return ERR_PTR(-EINVAL);
  
  	if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&

-   offset + size > mem->io_size &&
+   offset + size > resource_size(&mem->io) &&
!i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
return ERR_PTR(-ENOSPC);
  
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c

index 8c88075eeab2..d2440c793f84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct intel_memory_region 
*mem)
  
  	/* Exclude the reserved region from driver use */

mem->region.end = i915->dsm.reserved.start - 1;
-   mem->io_size = min(mem->io_size, resource_size(&mem->region));
+   mem->io = DEFINE_RES_MEM(mem->io.start,
+min(resource_size(&mem->io),
+resource_size(&mem->region)));
  
  	i915->dsm.usable_size = resource_size(&mem->region);
  
@@ -752,7 +754,7 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,

 * With discrete devices, where we lack a mappable aperture there is no
 * possible way to ever access this memory on the CPU side.
 */
-   if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size &&
+   if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !resource_size(&mem->io) 
&&
!(flags & I915_BO_ALLOC_GPU_ONLY))
return -ENOS

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

2023-12-12 Thread Ville Syrjälä 
On Wed, Dec 13, 2023 at 02:42:26AM +0200, Ville Syrjala wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c 
> b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> index 717c3a3237c4..1ac05d90b2e8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, 
> struct fb_info *info
>  
>   /* Use fbdev's framebuffer from lmem for discrete */
>   info->fix.smem_start =
> - (unsigned long)(mem->io_start +
> + (unsigned long)(mem->io.start +
>   i915_gem_object_get_dma_address(obj, 
> 0));

Hmm. That looks wrong for MTL actually since dma address is relative
to the start of LMEMBAR but stolen io.start will be LMEMBAR+8MiB (or
just DSMBASE which points to the same place, with the w/a in place).
So we need to subtract mem->region.start from this to get the correct
value.

-- 
Ville Syrjälä
Intel

[PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

2023-12-12 Thread Ville Syrjala 
From: Ville Syrjälä 

mem->region is a struct resource, but mem->io_start and
mem->io_size are not for whatever reason. Let's unify this
and convert the io stuff into a struct resource as well.
Should make life a little less annoying when you don't have
juggle between two different approaches all the time.

Mostly done using cocci (with manual tweaks at all the
places where we mutate io_size by hand):
@@
struct intel_memory_region *M;
expression START, SIZE;
@@
- M->io_start = START;
- M->io_size = SIZE;
+ M->io = DEFINE_RES_MEM(START, SIZE);

@@
struct intel_memory_region *M;
@@
- M->io_start
+ M->io.start

@@
struct intel_memory_region M;
@@
- M.io_start
+ M.io.start

@@
expression M;
@@
- M->io_size
+ resource_size(&M->io)

@@
expression M;
@@
- M.io_size
+ resource_size(&M.io)

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_region.c |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 17 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c|  8 
 .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +-
 drivers/gpu/drm/i915/gt/intel_region_lmem.c| 11 +++
 drivers/gpu/drm/i915/gt/selftest_tlb.c |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c  |  2 +-
 drivers/gpu/drm/i915/i915_query.c  |  2 +-
 drivers/gpu/drm/i915/intel_memory_region.c | 15 +++
 drivers/gpu/drm/i915/intel_memory_region.h |  3 +--
 drivers/gpu/drm/i915/intel_region_ttm.c|  8 
 .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
 13 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c 
b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
index 717c3a3237c4..1ac05d90b2e8 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
@@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, 
struct fb_info *info
 
/* Use fbdev's framebuffer from lmem for discrete */
info->fix.smem_start =
-   (unsigned long)(mem->io_start +
+   (unsigned long)(mem->io.start +
i915_gem_object_get_dma_address(obj, 
0));
info->fix.smem_len = obj->base.size;
} else {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index a4fb577eceb4..b09b74a2448b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct intel_memory_region 
*mem,
return ERR_PTR(-EINVAL);
 
if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&
-   offset + size > mem->io_size &&
+   offset + size > resource_size(&mem->io) &&
!i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
return ERR_PTR(-ENOSPC);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c 
b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 8c88075eeab2..d2440c793f84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct intel_memory_region 
*mem)
 
/* Exclude the reserved region from driver use */
mem->region.end = i915->dsm.reserved.start - 1;
-   mem->io_size = min(mem->io_size, resource_size(&mem->region));
+   mem->io = DEFINE_RES_MEM(mem->io.start,
+min(resource_size(&mem->io),
+resource_size(&mem->region)));
 
i915->dsm.usable_size = resource_size(&mem->region);
 
@@ -752,7 +754,7 @@ static int _i915_gem_object_stolen_init(struct 
intel_memory_region *mem,
 * With discrete devices, where we lack a mappable aperture there is no
 * possible way to ever access this memory on the CPU side.
 */
-   if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size &&
+   if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !resource_size(&mem->io) 
&&
!(flags & I915_BO_ALLOC_GPU_ONLY))
return -ENOSPC;
 
@@ -838,13 +840,12 @@ static int init_stolen_lmem(struct intel_memory_region 
*mem)
return 0;
}
 
-   if (mem->io_size &&
-   !io_mapping_init_wc(&mem->iomap, mem->io_start, mem->io_size))
+   if (resource_size(&mem->io) &&
+   !io_mapping_init_wc(&mem->iomap, mem->io.start, 
resource_size(&mem->io)))
goto err_cleanup;
 
-   drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
-   &mem->io_start);
-   drm_dbg(&i915->drm, "Stolen Local DSM base: %pa\n", &mem->region.start);
+   drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
+   drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
 
return