Re: [RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-14 Thread Nirmoy 


On 2/14/20 10:08 AM, Gerd Hoffmann wrote:

-   if (bo->mem.mm_node)
-   bo->offset = (bo->mem.start << PAGE_SHIFT) +
-   bdev->man[bo->mem.mem_type].gpu_offset;
-   else
-   bo->offset = 0;
-


My assumption is

  (bo->tbo.offset - slot->gpu_offset + offset) == (bo->tbo.mem.start << PAGE_SHIFT) + 
bdev->man[bo->mem.mem_type].gpu_offset - slot->gpu_offset + offset)

-> == (bo->tbo.mem.start << PAGE_SHIFT) + offset

That looks better.

Thanks, I will use that.



and we loose  slot->gpu_offset so I thought it should be

((bo->tbo.mem.start << PAGE_SHIFT) + slot->gpu_offset + offset);

No.


Yes  this doesn't work, qemu throws bunch of warnings. (tested without 
GUI, modprobe qxl)


[   10.691506] [drm] Device Version 0.0
[   10.691618] [drm] Compression level 0 log level 0
[   10.691759] [drm] 12286 io pages at offset 0x100
[   10.691897] [drm] 16777216 byte draw area at offset 0x0
[   10.692043] [drm] RAM header offset: 0x3ffe000
[   10.694823] [TTM] Zone  kernel: Available graphics memory: 240756 KiB
[   10.695055] [TTM] Initializing pool allocator
[   10.695294] [TTM] Initializing DMA pool allocator
[   10.695807] [drm] qxl: 16M of VRAM memory size
[   10.695933] [drm] qxl: 63M of IO pages memory ready (VRAM domain)
[   10.696093] [drm] qxl: 64M of Surface memory size
[   10.699969] [drm] slot 0 (main): base 0xf400, size 0x03ffe000, 
gpu_offset 0x0
[   10.700319] [drm] slot 1 (surfaces): base 0xf800, size 
0x0400, gpu_offset 0x100
[   10.707842] [drm] Initialized qxl 0.1.0 20120117 for :00:02.0 on 
minor 0
id 0, group 0, virt start 0, virt end , generation 0, 
delta 0
id 1, group 1, virt start 7f329f40, virt end 7f32a33fe000, 
generation 0, delta 7f329f40
id 2, group 1, virt start 7f329b00, virt end 7f329f00, 
generation 0, delta 7f329b00
qemu-system-x86_64: warning: Spice: memslot.c:64:memslot_validate_virt: 
virtual address out of range    virt=0x80329b30+0x4000 slot_id=2 
group_id=1

    slot=0x7f329b00-0x7f329f00 delta=0x7f329b00
qemu-system-x86_64: warning: Spice: 
display-channel.c:2437:display_channel_validate_surface: canvas address 
is 0x7f32d989eb18 for 0 (and is NULL)
qemu-system-x86_64: warning: Spice: 
display-channel.c:2439:display_channel_validate_surface: failed on 0
qemu-system-x86_64: warning: Spice: 
display-channel.c:2437:display_channel_validate_surface: canvas address 
is 0x7f32d989eb18 for 0 (and is NULL)
qemu-system-x86_64: warning: Spice: 
display-channel.c:2439:display_channel_validate_surface: failed on 0
qemu-system-x86_64: warning: Spice: 
display-channel.c:2437:display_channel_validate_surface: canvas address 
is 0x7f32d989eb18 for 0 (and is NULL)
qemu-system-x86_64: warning: Spice: 
display-channel.c:2439:display_channel_validate_surface: failed on 0
qemu-system-x86_64: warning: Spice: 
red-worker.c:468:destroy_primary_surface: double destroy of primary surface
id 0, group 0, virt start 0, virt end , generation 0, 
delta 0
id 1, group 1, virt start 7f329f40, virt end 7f32a33fe000, 
generation 0, delta 7f329f40
id 2, group 1, virt start 7f329b00, virt end 7f329f00, 
generation 0, delta 7f329b00
qemu-system-x86_64: warning: Spice: memslot.c:64:memslot_validate_virt: 
virtual address out of range    virt=0x80329b304000+0x30 slot_id=2 
group_id=1

    slot=0x7f329b00-0x7f329f00 delta=0x7f329b00
qemu-system-x86_64: warning: Spice: 
display-channel.c:2437:display_channel_validate_surface: canvas address 
is 0x7f32d989eb18 for 0 (and is NULL)
qemu-system-x86_64: warning: Spice: 
display-channel.c:2439:display_channel_validate_surface: failed on 0

[   10.723939] fbcon: qxldrmfb (fb0) is primary device
[   10.749245] Console: switching to colour frame buffer device 128x48
[   10.775038] qxl :00:02.0: fb0: qxldrmfb frame buffer device
qemu-system-x86_64: warning: Spice: 
display-channel.c:2437:display_channel_validate_surface: canvas address 
is 0x7f32d989eb18 for 0 (and is NULL)
qemu-system-x86_64: warning: Spice: 
display-channel.c:2439:display_channel_validate_surface: failed on 0


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-14 Thread Gerd Hoffmann 
> -   if (bo->mem.mm_node)
> -   bo->offset = (bo->mem.start << PAGE_SHIFT) +
> -   bdev->man[bo->mem.mem_type].gpu_offset;
> -   else
> -   bo->offset = 0;
> -
> 
> 
> My assumption is
> 
>  (bo->tbo.offset - slot->gpu_offset + offset) == (bo->tbo.mem.start << 
> PAGE_SHIFT) + bdev->man[bo->mem.mem_type].gpu_offset - slot->gpu_offset + 
> offset)
> 
> -> == (bo->tbo.mem.start << PAGE_SHIFT) + offset

That looks better.

> and we loose  slot->gpu_offset so I thought it should be
> 
> ((bo->tbo.mem.start << PAGE_SHIFT) + slot->gpu_offset + offset);

No.

The addressing scheme used by qxl is the slot in the high bits and the
offset within the slot in the low bits.  The qxl device has two pci
memory bars, the driver creates a slot for each of them, for ttm they
are VRAM and PRIV.

So maybe we don't need gpu_offset at all.  Not fully sure how driver and
ttm interact here.

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-14 Thread Nirmoy Das 
Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/qxl/qxl_drv.h| 6 ++
 drivers/gpu/drm/qxl/qxl_kms.c| 3 +++
 drivers/gpu/drm/qxl/qxl_object.h | 5 -
 drivers/gpu/drm/qxl/qxl_ttm.c| 9 -
 4 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 27e45a2d6b52..9a76a2a0283d 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -311,10 +311,8 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
qxl_bo *bo,
(bo->tbo.mem.mem_type == TTM_PL_VRAM)
? >main_slot : >surfaces_slot;
 
-   WARN_ON_ONCE((bo->tbo.offset & slot->gpu_offset) != slot->gpu_offset);
-
-   /* TODO - need to hold one of the locks to read tbo.offset */
-   return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
+   return slot->high_bits | ((bo->tbo.mem.start << PAGE_SHIFT) +
+ slot->gpu_offset + offset);
 }
 
 /* qxl_display.c */
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 611cbe7aee69..937cac9ba384 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -71,11 +71,14 @@ static void setup_slot(struct qxl_device *qdev,
   unsigned long size)
 {
uint64_t high_bits;
+   unsigned int gpu_offset_shift =
+   64 - (qdev->rom->slot_gen_bits + qdev->rom->slot_id_bits + 8);
 
slot->index = slot_index;
slot->name = slot_name;
slot->start_phys_addr = start_phys_addr;
slot->size = size;
+   slot->gpu_offset = (uint64_t)slot_index << gpu_offset_shift;
 
setup_hw_slot(qdev, slot);
 
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 8ae54ba7857c..21fa81048f4f 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -48,11 +48,6 @@ static inline void qxl_bo_unreserve(struct qxl_bo *bo)
ttm_bo_unreserve(>tbo);
 }
 
-static inline u64 qxl_bo_gpu_offset(struct qxl_bo *bo)
-{
-   return bo->tbo.offset;
-}
-
 static inline unsigned long qxl_bo_size(struct qxl_bo *bo)
 {
return bo->tbo.num_pages << PAGE_SHIFT;
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 16a5e903533d..2a43d0ef9ba1 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -56,11 +56,6 @@ static int qxl_invalidate_caches(struct ttm_bo_device *bdev, 
uint32_t flags)
 static int qxl_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 struct ttm_mem_type_manager *man)
 {
-   struct qxl_device *qdev = qxl_get_qdev(bdev);
-   unsigned int gpu_offset_shift =
-   64 - (qdev->rom->slot_gen_bits + qdev->rom->slot_id_bits + 8);
-   struct qxl_memslot *slot;
-
switch (type) {
case TTM_PL_SYSTEM:
/* System memory */
@@ -71,11 +66,7 @@ static int qxl_init_mem_type(struct ttm_bo_device *bdev, 
uint32_t type,
case TTM_PL_VRAM:
case TTM_PL_PRIV:
/* "On-card" video ram */
-   slot = (type == TTM_PL_VRAM) ?
-   >main_slot : >surfaces_slot;
-   slot->gpu_offset = (uint64_t)type << gpu_offset_shift;
man->func = _bo_manager_func;
-   man->gpu_offset = slot->gpu_offset;
man->flags = TTM_MEMTYPE_FLAG_FIXED |
 TTM_MEMTYPE_FLAG_MAPPABLE;
man->available_caching = TTM_PL_MASK_CACHING;
-- 
2.25.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-13 Thread Christian König 

Am 13.02.20 um 15:30 schrieb Gerd Hoffmann:

@@ -311,10 +311,8 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
qxl_bo *bo,
(bo->tbo.mem.mem_type == TTM_PL_VRAM)
? >main_slot : >surfaces_slot;
  
-	WARN_ON_ONCE((bo->tbo.offset & slot->gpu_offset) != slot->gpu_offset);

-
-   /* TODO - need to hold one of the locks to read tbo.offset */
-   return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
+   return slot->high_bits | ((bo->tbo.mem.start << PAGE_SHIFT) +
+ slot->gpu_offset + offset);
  }

--verbose please.


The warning and the TODO should probably stay, don't they?


I don't get the logic behind this change.


We ran into the problem that the whole offset handling in TTM is 
actually completely hardware specific.


E.g. Some need pfn, some need byte offsets, some need 32bit, some 64bit 
and some don't have a consistent offset at all (e.g. amdgpu for exmaple).


So we try to move that back into the drivers to concentrate on memory 
management in TTM.




The other chunks look sane, calculating slot->gpu_offset
in setup_slot() certainly makes sense.


Thanks for the review,
Christian.



cheers,
   Gerd



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-13 Thread Nirmoy 


On 2/13/20 3:30 PM, Gerd Hoffmann wrote:

@@ -311,10 +311,8 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
qxl_bo *bo,
(bo->tbo.mem.mem_type == TTM_PL_VRAM)
? >main_slot : >surfaces_slot;
  
-	WARN_ON_ONCE((bo->tbo.offset & slot->gpu_offset) != slot->gpu_offset);

-
-   /* TODO - need to hold one of the locks to read tbo.offset */
-   return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
+   return slot->high_bits | ((bo->tbo.mem.start << PAGE_SHIFT) +
+ slot->gpu_offset + offset);
  }

--verbose please.

I don't get the logic behind this change.


Hi Gerd,

I borrowed the logic for removed ttm part

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 229205e499db..2ccfebc3c9a2 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -382,12 +381,6 @@ static int ttm_bo_handle_move_mem(struct 
ttm_buffer_object *bo,

    bo->evicted = false;
    }

-   if (bo->mem.mm_node)
-   bo->offset = (bo->mem.start << PAGE_SHIFT) +
-   bdev->man[bo->mem.mem_type].gpu_offset;
-   else
-   bo->offset = 0;
-


My assumption is

 (bo->tbo.offset - slot->gpu_offset + offset) == (bo->tbo.mem.start << PAGE_SHIFT) + 
bdev->man[bo->mem.mem_type].gpu_offset - slot->gpu_offset + offset)

-> == (bo->tbo.mem.start << PAGE_SHIFT) + offset

and we loose  slot->gpu_offset so I thought it should be

((bo->tbo.mem.start << PAGE_SHIFT) + slot->gpu_offset + offset);

Can you please suggest me how to calculate the offset  correctly here.


Regards,

Nirmo



The other chunks look sane, calculating slot->gpu_offset
in setup_slot() certainly makes sense.

cheers,
   Gerd


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/6] drm/qxl: don't use ttm bo->offset

2020-02-13 Thread Gerd Hoffmann 
> @@ -311,10 +311,8 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
> qxl_bo *bo,
>   (bo->tbo.mem.mem_type == TTM_PL_VRAM)
>   ? >main_slot : >surfaces_slot;
>  
> - WARN_ON_ONCE((bo->tbo.offset & slot->gpu_offset) != slot->gpu_offset);
> -
> - /* TODO - need to hold one of the locks to read tbo.offset */
> - return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
> + return slot->high_bits | ((bo->tbo.mem.start << PAGE_SHIFT) +
> +   slot->gpu_offset + offset);
>  }

--verbose please.

I don't get the logic behind this change.

The other chunks look sane, calculating slot->gpu_offset
in setup_slot() certainly makes sense.

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel