Re: [Spice-devel] [PATCH 1/3] drm/qxl: add mode/framebuffer check functions

[Frediano Ziglio]

> 
> Add a helper functions to check video modes.  Also add a helper to check
> framebuffer buffer objects, using the former for consistency.  That way
> we should not fail in qxl_primary_atomic_check() because video modes
> which are too big will not be added to the mode list in the first place.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 37 +++--
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 1f8fddcc34..07a37d52c4 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -191,6 +191,21 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev)
>   }
>  }
>  
> +static int qxl_check_mode(struct qxl_device *qdev,
> +   unsigned int width,
> +   unsigned int height)
> +{
> + if (width * height * 4 > qdev->vram_size)

Is someone checking for integer overflows already?

> + return -ENOMEM;
> + return 0;
> +}
> +
> +static int qxl_check_framebuffer(struct qxl_device *qdev,
> +  struct qxl_bo *bo)
> +{
> + return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
> +}
> +
>  static int qxl_add_monitors_config_modes(struct drm_connector *connector,
>   unsigned *pwidth,
>   unsigned *pheight)
> @@ -466,12 +481,7 @@ static int qxl_primary_atomic_check(struct drm_plane
> *plane,
>  
>   bo = gem_to_qxl_bo(state->fb->obj[0]);
>  
> - if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
> - DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
> - return -EINVAL;
> - }
> -
> - return 0;
> + return qxl_check_framebuffer(qdev, bo);
>  }
>  
>  static int qxl_primary_apply_cursor(struct drm_plane *plane)
> @@ -941,20 +951,11 @@ static enum drm_mode_status qxl_conn_mode_valid(struct
> drm_connector *connector,
>  {
>   struct drm_device *ddev = connector->dev;
>   struct qxl_device *qdev = ddev->dev_private;
> - int i;
>  
> - /* TODO: is this called for user defined modes? (xrandr --add-mode)
> -  * TODO: check that the mode fits in the framebuffer */
> + if (qxl_check_mode(qdev, mode->hdisplay, mode->vdisplay) != 0)
> + return MODE_BAD;
>  
> - if (qdev->monitors_config_width == mode->hdisplay &&
> - qdev->monitors_config_height == mode->vdisplay)
> - return MODE_OK;
> -
> - for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
> - if (common_modes[i].w == mode->hdisplay && common_modes[i].h ==
> mode->vdisplay)
> - return MODE_OK;
> - }
> - return MODE_BAD;
> + return MODE_OK;
>  }
>  
>  static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector)

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

Re: [Spice-devel] [PATCH 4/4] drm/qxl: add drm_driver.release callback.

[Frediano Ziglio]

> 
> Move final cleanups to qxl_drm_release() callback.

Can you explain in the commit why this is better or preferable?

> Add drm_atomic_helper_shutdown() call to qxl_pci_remove().

I suppose this is to replace the former manual cleanup calls,
which were moved to qxl_drm_release, I think this could be
added in the commit message ("why"), I don't see much value
in describing "how" this was done.

> 
> Reorder calls in qxl_device_fini().  Cleaning up gem & ttm
> might trigger qxl commands, so we should do that before
> releaseing command rings.

Typo: releaseing -> releasing
Why not putting this in a separate commit? Was this behaviour
changed? It does not seem so to me.

> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 21 ++---
>  drivers/gpu/drm/qxl/qxl_kms.c |  8 
>  2 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 1d601f57a6ba..8044363ba0f2 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -34,6 +34,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -132,21 +133,25 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>   return ret;
>  }
>  
> +static void qxl_drm_release(struct drm_device *dev)
> +{
> + struct qxl_device *qdev = dev->dev_private;
> +
> + qxl_modeset_fini(qdev);
> + qxl_device_fini(qdev);
> + dev->dev_private = NULL;
> + kfree(qdev);
> +}
> +
>  static void
>  qxl_pci_remove(struct pci_dev *pdev)
>  {
>   struct drm_device *dev = pci_get_drvdata(pdev);
> - struct qxl_device *qdev = dev->dev_private;
>  
>   drm_dev_unregister(dev);
> -
> - qxl_modeset_fini(qdev);
> - qxl_device_fini(qdev);
> + drm_atomic_helper_shutdown(dev);
>   if (is_vga(pdev))
>   vga_put(pdev, VGA_RSRC_LEGACY_IO);
> -
> - dev->dev_private = NULL;
> - kfree(qdev);
>   drm_dev_put(dev);
>  }
>  
> @@ -279,6 +284,8 @@ static struct drm_driver qxl_driver = {
>   .major = 0,
>   .minor = 1,
>   .patchlevel = 0,
> +
> + .release = qxl_drm_release,
>  };
>  
>  static int __init qxl_init(void)
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index bfc1631093e9..70b20ee4741a 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -299,12 +299,12 @@ void qxl_device_fini(struct qxl_device *qdev)
>  {
>   qxl_bo_unref(&qdev->current_release_bo[0]);
>   qxl_bo_unref(&qdev->current_release_bo[1]);
> - flush_work(&qdev->gc_work);
> - qxl_ring_free(qdev->command_ring);
> - qxl_ring_free(qdev->cursor_ring);
> - qxl_ring_free(qdev->release_ring);
>   qxl_gem_fini(qdev);
>   qxl_bo_fini(qdev);
> + flush_work(&qdev->gc_work);
> + qxl_ring_free(qdev->command_ring);
> + qxl_ring_free(qdev->cursor_ring);
> + qxl_ring_free(qdev->release_ring);
>   io_mapping_free(qdev->surface_mapping);
>   io_mapping_free(qdev->vram_mapping);
>   iounmap(qdev->ram_header);

Frediano

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

Re: [Spice-devel] [PATCH 1/2] drm/qxl: stop abusing TTM to call driver internal functions

[Frediano Ziglio]

> 
> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> used in a driver.
> 

As far as I can see by your second patch QXL is just using exported
(that is not internal) functions.
Not that the idea of making them internal is bad but this comment is
a wrong statement.

> Instead call the qxl_ttm_io_mem_reserve() function directly.
> 

I would add that qxl_ttm_io_mem_free is empty so the removal of
ttm_mem_io_free is fine.

> Signed-off-by: Christian König 
> ---
>  drivers/gpu/drm/qxl/qxl_drv.h|  2 ++
>  drivers/gpu/drm/qxl/qxl_object.c | 11 +--
>  drivers/gpu/drm/qxl/qxl_ttm.c|  4 ++--
>  3 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 9e034c5fa87d..8a24f8e101da 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -354,6 +354,8 @@ int qxl_mode_dumb_mmap(struct drm_file *filp,
>  /* qxl ttm */
>  int qxl_ttm_init(struct qxl_device *qdev);
>  void qxl_ttm_fini(struct qxl_device *qdev);
> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> +struct ttm_mem_reg *mem);
>  int qxl_mmap(struct file *filp, struct vm_area_struct *vma);
>  
>  /* qxl image */
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> b/drivers/gpu/drm/qxl/qxl_object.c
> index 548dfe6f3b26..299e63a951c5 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -148,7 +148,6 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>  void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> struct qxl_bo *bo, int page_offset)
>  {
> - struct ttm_mem_type_manager *man =
> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
>   void *rptr;
>   int ret;
>   struct io_mapping *map;
> @@ -160,9 +159,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>   else
>   goto fallback;
>  
> - (void) ttm_mem_io_lock(man, false);
> - ret = ttm_mem_io_reserve(bo->tbo.bdev, &bo->tbo.mem);
> - ttm_mem_io_unlock(man);
> + ret = qxl_ttm_io_mem_reserve(bo->tbo.bdev, &bo->tbo.mem);
>  
>   return io_mapping_map_atomic_wc(map, bo->tbo.mem.bus.offset + 
> page_offset);
>  fallback:
> @@ -193,17 +190,11 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
>  void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
>  struct qxl_bo *bo, void *pmap)
>  {
> - struct ttm_mem_type_manager *man =
> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
> -
>   if ((bo->tbo.mem.mem_type != TTM_PL_VRAM) &&
>   (bo->tbo.mem.mem_type != TTM_PL_PRIV))
>   goto fallback;
>  
>   io_mapping_unmap_atomic(pmap);
> -
> - (void) ttm_mem_io_lock(man, false);
> - ttm_mem_io_free(bo->tbo.bdev, &bo->tbo.mem);
> - ttm_mem_io_unlock(man);
>   return;
>   fallback:
>   qxl_bo_kunmap(bo);
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index 9b24514c75aa..cb80e512dd46 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -159,8 +159,8 @@ static int qxl_verify_access(struct ttm_buffer_object
> *bo, struct file *filp)
> filp->private_data);
>  }
>  
> -static int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> -   struct ttm_mem_reg *mem)
> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> +struct ttm_mem_reg *mem)
>  {
>   struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>   struct qxl_device *qdev = qxl_get_qdev(bdev);

Otherwise fine for me.

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

Re: [Spice-devel] [PATCH 1/2] drm/qxl: stop abusing TTM to call driver internal functions

[Frediano Ziglio]

> 
> Am 27.09.19 um 18:31 schrieb Frediano Ziglio:
> >> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> >> used in a driver.
> >>
> > As far as I can see by your second patch QXL is just using exported
> > (that is not internal) functions.
> > Not that the idea of making them internal is bad but this comment is
> > a wrong statement.
> 
> See the history of exporting those, that was done specifically so that
> QXL can call them (commit afe6804c045fbd69a1b75c681107b5d6df9190de).
> 
> But those are the internal functions which TTM uses to call into the
> driver. That a driver uses them to call into itself doesn't seem to make
> sense.
> 

The commit was merged and release in Linux 3.10 6 years ago, since
then these functions have been exported. Not saying that the QXL change
was wrong and should not have been acked and merged but after 6 years
saying that these functions are internal it's not correct.

Something like

"The ttm_mem_io_* functions were intended to be internal to TTM and
shouldn't have been used in a driver. They were exported in commit
afe6804c045fbd69a1b75c681107b5d6df9190de just for QXL."

> >> Instead call the qxl_ttm_io_mem_reserve() function directly.
> >>
> > I would add that qxl_ttm_io_mem_free is empty so the removal of
> > ttm_mem_io_free is fine.
> 
> Good point, going to add that.
> 
> Thanks,
> Christian.
> 

Frediano

> >
> >> Signed-off-by: Christian König 
> >> ---
> >>   drivers/gpu/drm/qxl/qxl_drv.h|  2 ++
> >>   drivers/gpu/drm/qxl/qxl_object.c | 11 +--
> >>   drivers/gpu/drm/qxl/qxl_ttm.c|  4 ++--
> >>   3 files changed, 5 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> >> index 9e034c5fa87d..8a24f8e101da 100644
> >> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> >> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> >> @@ -354,6 +354,8 @@ int qxl_mode_dumb_mmap(struct drm_file *filp,
> >>   /* qxl ttm */
> >>   int qxl_ttm_init(struct qxl_device *qdev);
> >>   void qxl_ttm_fini(struct qxl_device *qdev);
> >> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> >> + struct ttm_mem_reg *mem);
> >>   int qxl_mmap(struct file *filp, struct vm_area_struct *vma);
> >>   
> >>   /* qxl image */
> >> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> >> b/drivers/gpu/drm/qxl/qxl_object.c
> >> index 548dfe6f3b26..299e63a951c5 100644
> >> --- a/drivers/gpu/drm/qxl/qxl_object.c
> >> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> >> @@ -148,7 +148,6 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
> >>   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> >>  struct qxl_bo *bo, int page_offset)
> >>   {
> >> -  struct ttm_mem_type_manager *man =
> >> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
> >>void *rptr;
> >>int ret;
> >>struct io_mapping *map;
> >> @@ -160,9 +159,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> >>else
> >>goto fallback;
> >>   
> >> -  (void) ttm_mem_io_lock(man, false);
> >> -  ret = ttm_mem_io_reserve(bo->tbo.bdev, &bo->tbo.mem);
> >> -  ttm_mem_io_unlock(man);
> >> +  ret = qxl_ttm_io_mem_reserve(bo->tbo.bdev, &bo->tbo.mem);
> >>   
> >>return io_mapping_map_atomic_wc(map, bo->tbo.mem.bus.offset +
> >>page_offset);
> >>   fallback:
> >> @@ -193,17 +190,11 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
> >>   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
> >>   struct qxl_bo *bo, void *pmap)
> >>   {
> >> -  struct ttm_mem_type_manager *man =
> >> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
> >> -
> >>if ((bo->tbo.mem.mem_type != TTM_PL_VRAM) &&
> >>(bo->tbo.mem.mem_type != TTM_PL_PRIV))
> >>goto fallback;
> >>   
> >>io_mapping_unmap_atomic(pmap);
> >> -
> >> -  (void) ttm_mem_io_lock(man, false);
> >> -  ttm_mem_io_free(bo->tbo.bdev, &bo->tbo.mem);
> >> -  ttm_mem_io_unlock(man);
> >>return;
> >>fallback:
> >>qxl_bo_kunmap(bo);
> >> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> >> index 9b24514c75aa..cb80e512dd46 100644
> >> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> >> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> >> @@ -159,8 +159,8 @@ static int qxl_verify_access(struct ttm_buffer_object
> >> *bo, struct file *filp)
> >>  filp->private_data);
> >>   }
> >>   
> >> -static int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> >> -struct ttm_mem_reg *mem)
> >> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> >> + struct ttm_mem_reg *mem)
> >>   {
> >>struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
> >>struct qxl_device *qdev = qxl_get_qdev(bdev);
> > Otherwise fine for me.
> >
> > Frediano
> 
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Spice-devel] Xorg indefinitely hangs in kernelspace

[Frediano Ziglio]

> 
> On 05.09.19 15:34, Jaak Ristioja wrote:
> > On 05.09.19 10:14, Gerd Hoffmann wrote:
> >> On Tue, Aug 06, 2019 at 09:00:10PM +0300, Jaak Ristioja wrote:
> >>> Hello!
> >>>
> >>> I'm writing to report a crash in the QXL / DRM code in the Linux kernel.
> >>> I originally filed the issue on LaunchPad and more details can be found
> >>> there, although I doubt whether these details are useful.
> >>
> >> Any change with kernel 5.3-rc7 ?
> > 
> > Didn't try. Did you change something? I could try, but I've done so
> > before and every time this bug manifests itself with MAJOR.MINOR-rc# I
> > get asked to try version MAJOR.(MINOR+1)-rc# so I guess I could as well
> > give up?
> > 
> > Alright, I'll install 5.3-rc7, but once more it might take some time for
> > this bug to expose itself.
> 
> Just got the issue with 5.3.0-050300rc7-generic:
> 
> [124212.547403] INFO: task Xorg:797 blocked for more than 120 seconds.
> [124212.548639]   Not tainted 5.3.0-050300rc7-generic #201909021831
> [124212.549839] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [124212.547403] INFO: task Xorg:797 blocked for more than 120 seconds.
> [124212.548639]   Not tainted 5.3.0-050300rc7-generic #201909021831
> [124212.549839] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [124212.551329] XorgD0   797773 0x00404004
> [124212.551331] Call Trace:
> [124212.551336]  __schedule+0x2b9/0x6c0
> [124212.551337]  schedule+0x42/0xb0
> [124212.551338]  schedule_preempt_disabled+0xe/0x10
> [124212.551340]  __ww_mutex_lock.isra.0+0x261/0x7f0
> [124212.551345]  ? ttm_bo_init+0x6b/0x100 [ttm]
> [124212.551346]  __ww_mutex_lock_slowpath+0x16/0x20
> [124212.551347]  ww_mutex_lock+0x38/0x90
> [124212.551352]  ttm_eu_reserve_buffers+0x1cc/0x2f0 [ttm]
> [124212.551371]  qxl_release_reserve_list+0x6d/0x150 [qxl]
> [124212.551373]  ? qxl_bo_pin+0xf4/0x190 [qxl]
> [124212.551375]  qxl_cursor_atomic_update+0x1ab/0x2e0 [qxl]
> [124212.551376]  ? qxl_bo_pin+0xf4/0x190 [qxl]
> [124212.551384]  drm_atomic_helper_commit_planes+0xd5/0x220 [drm_kms_helper]
> [124212.551388]  drm_atomic_helper_commit_tail+0x2c/0x70 [drm_kms_helper]
> [124212.551392]  commit_tail+0x68/0x70 [drm_kms_helper]
> [124212.551395]  drm_atomic_helper_commit+0x118/0x120 [drm_kms_helper]
> [124212.551407]  drm_atomic_commit+0x4a/0x50 [drm]
> [124212.551411]  drm_atomic_helper_update_plane+0xea/0x100 [drm_kms_helper]
> [124212.551418]  __setplane_atomic+0xcb/0x110 [drm]
> [124212.551428]  drm_mode_cursor_universal+0x140/0x260 [drm]
> [124212.551435]  drm_mode_cursor_common+0xcc/0x220 [drm]
> [124212.551441]  ? drm_mode_cursor_ioctl+0x60/0x60 [drm]
> [124212.551447]  drm_mode_cursor2_ioctl+0xe/0x10 [drm]
> [124212.551452]  drm_ioctl_kernel+0xae/0xf0 [drm]
> [124212.551458]  drm_ioctl+0x234/0x3d0 [drm]
> [124212.551464]  ? drm_mode_cursor_ioctl+0x60/0x60 [drm]
> [124212.551466]  ? timerqueue_add+0x5f/0xa0
> [124212.551469]  ? enqueue_hrtimer+0x3d/0x90
> [124212.551471]  do_vfs_ioctl+0x407/0x670
> [124212.551473]  ? fput+0x13/0x20
> [124212.551475]  ? __sys_recvmsg+0x88/0xa0
> [124212.551476]  ksys_ioctl+0x67/0x90
> [124212.551477]  __x64_sys_ioctl+0x1a/0x20
> [124212.551479]  do_syscall_64+0x5a/0x130
> [124212.551480]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [124212.551481] RIP: 0033:0x7f07c79ee417
> [124212.551485] Code: Bad RIP value.
> [124212.551485] RSP: 002b:7ffc6b1de1a8 EFLAGS: 3246 ORIG_RAX:
> 0010
> [124212.551486] RAX: ffda RBX: 5612f109a610 RCX:
> 7f07c79ee417
> [124212.551487] RDX: 7ffc6b1de1e0 RSI: c02464bb RDI:
> 000e
> [124212.551487] RBP: 7ffc6b1de1e0 R08: 0040 R09:
> 0004
> [124212.551488] R10: 003f R11: 3246 R12:
> c02464bb
> [124212.551488] R13: 000e R14:  R15:
> 5612f10981d0
> [124333.376328] INFO: task Xorg:797 blocked for more than 241 seconds.
> [124333.377474]   Not tainted 5.3.0-050300rc7-generic #201909021831
> [124333.378609] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [124333.376328] INFO: task Xorg:797 blocked for more than 241 seconds.
> [124333.377474]   Not tainted 5.3.0-050300rc7-generic #201909021831
> [124333.378609] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [124333.380060] XorgD0   797773 0x00404004
> [124333.380062] Call Trace:
> [124333.380067]  __schedule+0x2b9/0x6c0
> [124333.380068]  schedule+0x42/0xb0
> [124333.380069]  schedule_preempt_disabled+0xe/0x10
> [124333.380070]  __ww_mutex_lock.isra.0+0x261/0x7f0
> [124333.380076]  ? ttm_bo_init+0x6b/0x100 [ttm]
> [124333.380077]  __ww_mutex_lock_slowpath+0x16/0x20
> [124333.380077]  ww_mutex_lock+0x38/0x90
> [124333.380080]  ttm_eu_reserve_buffers+0x1cc/0x2f0 [ttm]
> [124333.380083]  qxl_release_reserve_list+0x6d/0x150 [qxl]
> [124333.380085]  ? qxl_bo_pin+0xf4/0x190 [q

Re: [Spice-devel] [PATCH 1/2] drm/qxl: stop abusing TTM to call driver internal functions

[Frediano Ziglio]

> Am 30.09.19 um 11:51 schrieb Frediano Ziglio:
> >> Am 27.09.19 um 18:31 schrieb Frediano Ziglio:
> >>>> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> >>>> used in a driver.
> >>>>
> >>> As far as I can see by your second patch QXL is just using exported
> >>> (that is not internal) functions.
> >>> Not that the idea of making them internal is bad but this comment is
> >>> a wrong statement.
> >> See the history of exporting those, that was done specifically so that
> >> QXL can call them (commit afe6804c045fbd69a1b75c681107b5d6df9190de).
> >>
> >> But those are the internal functions which TTM uses to call into the
> >> driver. That a driver uses them to call into itself doesn't seem to make
> >> sense.
> >>
> > The commit was merged and release in Linux 3.10 6 years ago, since
> > then these functions have been exported. Not saying that the QXL change
> > was wrong and should not have been acked and merged but after 6 years
> > saying that these functions are internal it's not correct.
> 
> Why? If a function is internal or not is defined by the design and not
> the actual implementation.
> 

Where's documented? I cannot find it. Probably my kernel devel is a bit
rusty.

> > Something like
> >
> > "The ttm_mem_io_* functions were intended to be internal to TTM and
> > shouldn't have been used in a driver. They were exported in commit
> > afe6804c045fbd69a1b75c681107b5d6df9190de just for QXL."
> 
> Good point mentioning the commit adding that, going to use this for the
> commit message.
> 
> Christian.
> 
> >
> >>>> Instead call the qxl_ttm_io_mem_reserve() function directly.
> >>>>
> >>> I would add that qxl_ttm_io_mem_free is empty so the removal of
> >>> ttm_mem_io_free is fine.
> >> Good point, going to add that.
> >>
> >> Thanks,
> >> Christian.
> >>
> > Frediano
> >
> >>>> Signed-off-by: Christian König 
> >>>> ---
> >>>>drivers/gpu/drm/qxl/qxl_drv.h|  2 ++
> >>>>drivers/gpu/drm/qxl/qxl_object.c | 11 +--
> >>>>drivers/gpu/drm/qxl/qxl_ttm.c|  4 ++--
> >>>>3 files changed, 5 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h
> >>>> b/drivers/gpu/drm/qxl/qxl_drv.h
> >>>> index 9e034c5fa87d..8a24f8e101da 100644
> >>>> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> >>>> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> >>>> @@ -354,6 +354,8 @@ int qxl_mode_dumb_mmap(struct drm_file *filp,
> >>>>/* qxl ttm */
> >>>>int qxl_ttm_init(struct qxl_device *qdev);
> >>>>void qxl_ttm_fini(struct qxl_device *qdev);
> >>>> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> >>>> +   struct ttm_mem_reg *mem);
> >>>>int qxl_mmap(struct file *filp, struct vm_area_struct *vma);
> >>>>
> >>>>/* qxl image */
> >>>> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> >>>> b/drivers/gpu/drm/qxl/qxl_object.c
> >>>> index 548dfe6f3b26..299e63a951c5 100644
> >>>> --- a/drivers/gpu/drm/qxl/qxl_object.c
> >>>> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> >>>> @@ -148,7 +148,6 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
> >>>>void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> >>>>struct qxl_bo *bo, int page_offset)
> >>>>{
> >>>> -struct ttm_mem_type_manager *man =
> >>>> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
> >>>>  void *rptr;
> >>>>  int ret;
> >>>>  struct io_mapping *map;
> >>>> @@ -160,9 +159,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device
> >>>> *qdev,
> >>>>  else
> >>>>  goto fallback;
> >>>>
> >>>> -(void) ttm_mem_io_lock(man, false);
> >>>> -ret = ttm_mem_io_reserve(bo->tbo.bdev, &bo->tbo.mem);
> >>>> -ttm_mem_io_unlock(man);
> >>>> +ret = qxl_ttm_io_mem_reserve(bo->tbo.bdev, &bo->tbo.mem);
> >>>>
> >>>>  return io_mapping_map_a

Re: [Spice-devel] Xorg indefinitely hangs in kernelspace

[Frediano Ziglio]

> 
> On Tue, 6 Aug 2019 21:00:10 +0300 From:   Jaak Ristioja 
> > Hello!
> > 
> > I'm writing to report a crash in the QXL / DRM code in the Linux kernel.
> > I originally filed the issue on LaunchPad and more details can be found
> > there, although I doubt whether these details are useful.
> > 
> >   https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1813620
> > 
> > I first experienced these issues with:
> > 
> > * Ubuntu 18.04 (probably kernel 4.15.something)
> > * Ubuntu 18.10 (kernel 4.18.0-13)
> > * Ubuntu 19.04 (kernel 5.0.0-13-generic)
> > * Ubuntu 19.04 (mainline kernel 5.1-rc7)
> > * Ubuntu 19.04 (mainline kernel 5.2.0-050200rc1-generic)
> > 
> > Here is the crash output from dmesg:
> > 
> > [354073.713350] INFO: task Xorg:920 blocked for more than 120 seconds.
> > [354073.717755]   Not tainted 5.2.0-050200rc1-generic #201905191930
> > [354073.722277] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> > disables this message.
> > [354073.738332] XorgD0   920854 0x00404004
> > [354073.738334] Call Trace:
> > [354073.738340]  __schedule+0x2ba/0x650
> > [354073.738342]  schedule+0x2d/0x90
> > [354073.738343]  schedule_preempt_disabled+0xe/0x10
> > [354073.738345]  __ww_mutex_lock.isra.11+0x3e0/0x750
> > [354073.738346]  __ww_mutex_lock_slowpath+0x16/0x20
> > [354073.738347]  ww_mutex_lock+0x34/0x50
> > [354073.738352]  ttm_eu_reserve_buffers+0x1f9/0x2e0 [ttm]
> > [354073.738356]  qxl_release_reserve_list+0x67/0x150 [qxl]
> > [354073.738358]  ? qxl_bo_pin+0xaa/0x190 [qxl]
> > [354073.738359]  qxl_cursor_atomic_update+0x1b0/0x2e0 [qxl]
> > [354073.738367]  drm_atomic_helper_commit_planes+0xb9/0x220
> > [drm_kms_helper]
> > [354073.738371]  drm_atomic_helper_commit_tail+0x2b/0x70 [drm_kms_helper]
> > [354073.738374]  commit_tail+0x67/0x70 [drm_kms_helper]
> > [354073.738378]  drm_atomic_helper_commit+0x113/0x120 [drm_kms_helper]
> > [354073.738390]  drm_atomic_commit+0x4a/0x50 [drm]
> > [354073.738394]  drm_atomic_helper_update_plane+0xe9/0x100 [drm_kms_helper]
> > [354073.738402]  __setplane_atomic+0xd3/0x120 [drm]
> > [354073.738410]  drm_mode_cursor_universal+0x142/0x270 [drm]
> > [354073.738418]  drm_mode_cursor_common+0xcb/0x220 [drm]
> > [354073.738425]  ? drm_mode_cursor_ioctl+0x60/0x60 [drm]
> > [354073.738432]  drm_mode_cursor2_ioctl+0xe/0x10 [drm]
> > [354073.738438]  drm_ioctl_kernel+0xb0/0x100 [drm]
> > [354073.738440]  ? ___sys_recvmsg+0x16c/0x200
> > [354073.738445]  drm_ioctl+0x233/0x410 [drm]
> > [354073.738452]  ? drm_mode_cursor_ioctl+0x60/0x60 [drm]
> > [354073.738454]  ? timerqueue_add+0x57/0x90
> > [354073.738456]  ? enqueue_hrtimer+0x3c/0x90
> > [354073.738458]  do_vfs_ioctl+0xa9/0x640
> > [354073.738459]  ? fput+0x13/0x20
> > [354073.738461]  ? __sys_recvmsg+0x88/0xa0
> > [354073.738462]  ksys_ioctl+0x67/0x90
> > [354073.738463]  __x64_sys_ioctl+0x1a/0x20
> > [354073.738465]  do_syscall_64+0x5a/0x140
> > [354073.738467]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [354073.738468] RIP: 0033:0x7ffad14d3417
> > [354073.738472] Code: Bad RIP value.
> > [354073.738472] RSP: 002b:7ffdd5679978 EFLAGS: 3246 ORIG_RAX:
> > 0010
> > [354073.738473] RAX: ffda RBX: 56428a474610 RCX:
> > 7ffad14d3417
> > [354073.738474] RDX: 7ffdd56799b0 RSI: c02464bb RDI:
> > 000e
> > [354073.738474] RBP: 7ffdd56799b0 R08: 0040 R09:
> > 0010
> > [354073.738475] R10: 003f R11: 3246 R12:
> > c02464bb
> > [354073.738475] R13: 000e R14:  R15:
> > 56428a4721d0
> > [354073.738511] INFO: task kworker/1:0:27625 blocked for more than 120
> > seconds.
> > [354073.745154]   Not tainted 5.2.0-050200rc1-generic #201905191930
> > [354073.751900] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> > disables this message.
> > [354073.762197] kworker/1:0 D0 27625  2 0x80004000
> > [354073.762205] Workqueue: events qxl_client_monitors_config_work_func
> > [qxl]
> > [354073.762206] Call Trace:
> > [354073.762211]  __schedule+0x2ba/0x650
> > [354073.762214]  schedule+0x2d/0x90
> > [354073.762215]  schedule_preempt_disabled+0xe/0x10
> > [354073.762216]  __ww_mutex_lock.isra.11+0x3e0/0x750
> > [354073.762217]  ? __switch_to_asm+0x34/0x70
> > [354073.762218]  ? __switch_to_asm+0x40/0x70
> > [354073.762219]  ? __switch_to_asm+0x40/0x70
> > [354073.762220]  __ww_mutex_lock_slowpath+0x16/0x20
> > [354073.762221]  ww_mutex_lock+0x34/0x50
> > [354073.762235]  drm_modeset_lock+0x35/0xb0 [drm]
> > [354073.762243]  drm_modeset_lock_all_ctx+0x5d/0xe0 [drm]
> > [354073.762251]  drm_modeset_lock_all+0x5e/0xb0 [drm]
> > [354073.762252]  qxl_display_read_client_monitors_config+0x1e1/0x370 [qxl]
> > [354073.762254]  qxl_client_monitors_config_work_func+0x15/0x20 [qxl]
> > [354073.762256]  process_one_work+0x20f/0x410
> > [354073.762257]  worker_thread+0x34/0x400
> > [354073.762259]  kthread+0x120/0x140
> > [354073.762260]  ? process_one_work+0x4

Re: [PATCH v2] drm/qxl: get vga ioports

[Frediano Ziglio]

> 
> qxl has two modes: "native" (used by the drm driver) and "vga" (vga
> compatibility mode, typically used for boot display and firmware
> framebuffers).
> 
> Accessing any vga ioport will switch the qxl device into vga mode.
> The qxl driver never does that, but other drivers accessing vga ports
> can trigger that too and therefore disturb qxl operation.  So aquire
> the legacy vga ioports from vgaarb to avoid that.
> 
> Reproducer: Boot kvm guest with both qxl and i915 vgpu, with qxl being
> first in pci scan order.
> 
> v2: Skip this for secondary qxl cards which don't have vga mode in the
> first place (Frediano).
> 
> Cc: Frediano Ziglio 
> Signed-off-by: Gerd Hoffmann 

Acked-by: Frediano Ziglio 

> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index b57a37543613..fcb48ac60598 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -63,6 +63,11 @@ module_param_named(num_heads, qxl_num_crtc, int, 0400);
>  static struct drm_driver qxl_driver;
>  static struct pci_driver qxl_pci_driver;
>  
> +static bool is_vga(struct pci_dev *pdev)
> +{
> + return pdev->class == PCI_CLASS_DISPLAY_VGA << 8;
> +}
> +
>  static int
>  qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
> @@ -87,9 +92,17 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>   if (ret)
>   goto disable_pci;
>  
> + if (is_vga(pdev)) {
> + ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
> + if (ret) {
> + DRM_ERROR("can't get legacy vga ioports\n");
> + goto disable_pci;
> + }
> + }
> +
>   ret = qxl_device_init(qdev, &qxl_driver, pdev);
>   if (ret)
> - goto disable_pci;
> + goto put_vga;
>  
>   ret = qxl_modeset_init(qdev);
>   if (ret)
> @@ -109,6 +122,9 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>   qxl_modeset_fini(qdev);
>  unload:
>   qxl_device_fini(qdev);
> +put_vga:
> + if (is_vga(pdev))
> + vga_put(pdev, VGA_RSRC_LEGACY_IO);
>  disable_pci:
>   pci_disable_device(pdev);
>  free_dev:
> @@ -126,6 +142,8 @@ qxl_pci_remove(struct pci_dev *pdev)
>  
>   qxl_modeset_fini(qdev);
>   qxl_device_fini(qdev);
> + if (is_vga(pdev))
> + vga_put(pdev, VGA_RSRC_LEGACY_IO);
>  
>   dev->dev_private = NULL;
>   kfree(qdev);

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: get vga ioports

[Frediano Ziglio]

> 
> qxl has two modes: "native" (used by the drm driver) and "vga" (vga
> compatibility mode, typically used for boot display and firmware
> framebuffers).
> 
> Accessing any vga ioport will switch the qxl device into vga mode.
> The qxl driver never does that, but other drivers accessing vga ports
> can trigger that too and therefore disturb qxl operation.  So aquire
> the legacy vga ioports from vgaarb to avoid that.
> 
> Reporducer: Boot kvm guest with both qxl and i915 vgpu, with qxl being

typo: "Reporducer"

> first in pci scan order.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index b57a37543613..8a2e86adc423 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -87,9 +87,15 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>   if (ret)
>   goto disable_pci;
>  
> + ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
> + if (ret) {
> + DRM_ERROR("can't get legacy vga ports\n");
> + goto put_vga;

I suppose that if this fails it's secondary so should continue.
What happen configuring 2 QXL devices?
Only a card should provide VGA registers in the system so
if any other card provide them QXL won't work.

> + }
> +
>   ret = qxl_device_init(qdev, &qxl_driver, pdev);
>   if (ret)
> - goto disable_pci;
> + goto put_vga;
>  
>   ret = qxl_modeset_init(qdev);
>   if (ret)
> @@ -109,6 +115,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
>   qxl_modeset_fini(qdev);
>  unload:
>   qxl_device_fini(qdev);
> +put_vga:
> + vga_put(pdev, VGA_RSRC_LEGACY_IO);

What happen if you didn't get the I/O? Maybe it's safe to
just call vga_put and avoid adding an additional label here?

>  disable_pci:
>   pci_disable_device(pdev);
>  free_dev:
> @@ -126,6 +134,7 @@ qxl_pci_remove(struct pci_dev *pdev)
>  
>   qxl_modeset_fini(qdev);
>   qxl_device_fini(qdev);
> + vga_put(pdev, VGA_RSRC_LEGACY_IO);
>  
>   dev->dev_private = NULL;
>   kfree(qdev);

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

[Spice-devel] [drm/qxl v3 5/7] qxl: Remove qxl_bo_init() return value

[Frediano Ziglio]

> 
> It's always returning 0, and it's always ignored.

Missing the Signed-off-by.

Acked-by: Frediano Ziglio 

You should add the Acked-by message when posting new
versions of the patch series.

Frediano


> ---
>  drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
>  drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 5a4720a..feac7e6 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
>  int qxl_destroy_monitors_object(struct qxl_device *qdev);
>  
>  /* qxl_gem.c */
> -int qxl_gem_init(struct qxl_device *qdev);
> +void qxl_gem_init(struct qxl_device *qdev);
>  void qxl_gem_fini(struct qxl_device *qdev);
>  int qxl_gem_object_create(struct qxl_device *qdev, int size,
> int alignment, int initial_domain,
> diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
> index d9746e9..3f185c4 100644
> --- a/drivers/gpu/drm/qxl/qxl_gem.c
> +++ b/drivers/gpu/drm/qxl/qxl_gem.c
> @@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
>  {
>  }
>  
> -int qxl_gem_init(struct qxl_device *qdev)
> +void qxl_gem_init(struct qxl_device *qdev)
>  {
>   INIT_LIST_HEAD(&qdev->gem.objects);
> - return 0;
>  }
>  
>  void qxl_gem_fini(struct qxl_device *qdev)

[Spice-devel] [drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Frediano Ziglio]

> 
> When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> interrupt,
> we currently always notify userspace that there was some hotplug event.
> 
> However, gnome-shell/mutter is reacting to this event by attempting a
> resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
> and then drmModeSetCrtc. This has the side-effect of causing
> qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
> surface was destroyed and created. After going through QEMU and then the
> remote SPICE client, a new identical monitors config message will be
> sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
> be emitted, and the same scenario occurring again.
> 
> As destroying/creating the primary surface causes a visible screen
> flicker, this makes the guest hard to use (
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).
> 
> This commit checks if the screen configuration we received is the same
> one as the current one, and does not notify userspace about it if that's
> the case.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 62
>  ---
>  1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 8cf5177..518333c 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct
> qxl_device *qdev, unsigned c
>   qdev->client_monitors_config->count = count;
>  }
>  
> +enum {
> + MONITORS_CONFIG_MODIFIED,
> + MONITORS_CONFIG_UNCHANGED,
> + MONITORS_CONFIG_BAD_CRC,
> +};
> +
>  static int qxl_display_copy_rom_client_monitors_config(struct qxl_device
>  *qdev)
>  {
>   int i;
>   int num_monitors;
>   uint32_t crc;
> + int status = MONITORS_CONFIG_UNCHANGED;
>  
>   num_monitors = qdev->rom->client_monitors_config.count;
>   crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
> @@ -70,7 +77,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
>  sizeof(qdev->rom->client_monitors_config),
>  qdev->rom->client_monitors_config_crc);
> - return 1;
> + return MONITORS_CONFIG_BAD_CRC;
>   }
>   if (num_monitors > qdev->monitors_config->max_allowed) {
>   DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
> %d\n",
> @@ -79,6 +86,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   } else {
>   num_monitors = qdev->rom->client_monitors_config.count;
>   }
> + if (qdev->client_monitors_config
> +   && (num_monitors != qdev->client_monitors_config->count)) {
> + status = MONITORS_CONFIG_MODIFIED;
> + }
>   qxl_alloc_client_monitors_config(qdev, num_monitors);
>   /* we copy max from the client but it isn't used */
>   qdev->client_monitors_config->max_allowed =
> @@ -88,17 +99,39 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   &qdev->rom->client_monitors_config.heads[i];
>   struct qxl_head *client_head =
>   &qdev->client_monitors_config->heads[i];
> - client_head->x = c_rect->left;
> - client_head->y = c_rect->top;
> - client_head->width = c_rect->right - c_rect->left;
> - client_head->height = c_rect->bottom - c_rect->top;
> - client_head->surface_id = 0;
> - client_head->id = i;
> - client_head->flags = 0;
> + if (client_head->x != c_rect->left) {
> + client_head->x = c_rect->left;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->y != c_rect->top) {
> + client_head->y = c_rect->top;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->width != c_rect->right - c_rect->left) {
> + client_head->width = c_rect->right - c_rect->left;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->height != c_rect->bottom - c_rect->top) {
> + client_head->height = c_rect->bottom - c_rect->top;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->surface_id != 0) {
> + client_head->surface_id = 0;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->id != i) {
> + client_head->id = i;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + 

[Spice-devel] [PATCH drm/qxl v4 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Frediano Ziglio]

_rect->bottom - c_rect->top) {
> + client_head->height = c_rect->bottom - c_rect->top;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->surface_id != 0) {
> + client_head->surface_id = 0;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->id != i) {
> + client_head->id = i;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->flags != 0) {
> + client_head->flags = 0;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
>   DRM_DEBUG_KMS("read %dx%d+%d+%d\n", client_head->width,
>   client_head->height,
> client_head->x, client_head->y);
>   }
> - return 0;
> +
> + return status;
>  }
>  
>  static void qxl_update_offset_props(struct qxl_device *qdev)
> @@ -124,9 +157,18 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev)
>  {
>  
>   struct drm_device *dev = qdev->ddev;
> - while (qxl_display_copy_rom_client_monitors_config(qdev)) {
> + int status;
> +
> + status = qxl_display_copy_rom_client_monitors_config(qdev);
> + while (status == MONITORS_CONFIG_BAD_CRC) {
>   qxl_io_log(qdev, "failed crc check for client_monitors_config,"
>" retrying\n");
> + status = qxl_display_copy_rom_client_monitors_config(qdev);
> + }
> + if (status == MONITORS_CONFIG_UNCHANGED) {
> + qxl_io_log(qdev, "config unchanged\n");
> + DRM_DEBUG("ignoring unchanged client monitors config");
> + return;
>   }
>  
>   drm_modeset_lock_all(dev);

Acked-by: Frediano Ziglio 

Frediano

[Spice-devel] [drm/qxl 1/6] qxl: Mark some internal functions as static

[Frediano Ziglio]

> 
> They are not used outside of their respective source file
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
>  drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
>  drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
> index 04270f5..74fc936 100644
> --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> @@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
>   return 0;
>  }
>  
> -int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
> +static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
>  {
>   struct qxl_rect rect;
>   int ret;
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index a61c0d4..156b7de 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
>   return head->width && head->height;
>  }
>  
> -void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned
> count)
> +static void qxl_alloc_client_monitors_config(struct qxl_device *qdev,
> unsigned count)
>  {
>   if (qdev->client_monitors_config &&
>   count > qdev->client_monitors_config->count) {
> @@ -607,7 +607,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
>   return true;
>  }
>  
> -void
> +static void
>  qxl_send_monitors_config(struct qxl_device *qdev)
>  {
>   int i;
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 5f3e5ad..da41e1f 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -395,13 +395,11 @@ qxl_framebuffer_init(struct drm_device *dev,
>struct drm_gem_object *obj,
>const struct drm_framebuffer_funcs *funcs);
>  void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
> -void qxl_send_monitors_config(struct qxl_device *qdev);
>  int qxl_create_monitors_object(struct qxl_device *qdev);
>  int qxl_destroy_monitors_object(struct qxl_device *qdev);
>  
>  /* used by qxl_debugfs only */
>  void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
> -void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned
> count);
>  
>  /* qxl_gem.c */
>  int qxl_gem_init(struct qxl_device *qdev);
> @@ -574,6 +572,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct
> qxl_bo *bo);
>  struct qxl_drv_surface *
>  qxl_surface_lookup(struct drm_device *dev, int surface_id);
>  void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool
>  freeing);
> -int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);
>  
>  #endif

Frediano

[Spice-devel] [drm/qxl 3/6] qxl: Add missing '\n' to qxl_io_log() call

[Frediano Ziglio]

> 
> The message has to be terminated by a newline as it's not going to get
> added automatically.
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> index 2cd879a..0d16107 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -197,7 +197,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer
> *fb,
>   /*
>* we are using a shadow draw buffer, at qdev->surface0_shadow
>*/
> - qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
> + qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
>  clips->y1, clips->y2);
>   image->dx = clips->x1;
>   image->dy = clips->y1;

Frediano

[Spice-devel] [drm/qxl 2/6] qxl: Remove unused prototype

[Frediano Ziglio]

> 
> qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
> implemented.
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index da41e1f..590ba25 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -398,9 +398,6 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev);
>  int qxl_create_monitors_object(struct qxl_device *qdev);
>  int qxl_destroy_monitors_object(struct qxl_device *qdev);
>  
> -/* used by qxl_debugfs only */
> -void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
> -
>  /* qxl_gem.c */
>  int qxl_gem_init(struct qxl_device *qdev);
>  void qxl_gem_fini(struct qxl_device *qdev);

Frediano

[Spice-devel] [drm/qxl 6/6] qxl: Allow resolution which are not multiple of 8

[Frediano Ziglio]

> 
> The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
> the resolutions we are going to present to user-space are going to be
> rounded down to a multiple of 8. In the QXL arbitrary resolution case,
> this is not useful.
> This commit forces the actual width/height that was requested by the
> client in the drm_display_mode structure rather than keeping the
> rounded version.
> 
> Signed-off-by: Christophe Fergeau 
> ---
> 
> I know this is very hacky, but I have no idea what is important to be set in
> the mode struct,
> if there is a better way to create it without getting the rounding to a
> multiple of 8, ...
> 
> 
>  drivers/gpu/drm/qxl/qxl_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index edb90f6..fc5b01e 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -202,6 +202,9 @@ static int qxl_add_monitors_config_modes(struct
> drm_connector *connector,
>   mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
>   false);
>   mode->type |= DRM_MODE_TYPE_PREFERRED;
> + mode->hdisplay = head->width;
> + mode->vdisplay = head->height;
> + drm_mode_set_name(mode);
>   *pwidth = head->width;
>   *pheight = head->height;
>   drm_mode_probed_add(connector, mode);

If I remember I was discussing with Pavel some time ago about the implementation
of this.
And if I remember he tested some code.
Pavel, do you remember something about?

Frediano

[Spice-devel] [drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Frediano Ziglio]

> 
> qdev->gem.objects was initialized directly in qxl_device_init() rather
> than going through qxl_gem_init(), and qxl_gem_fini() was never called.
> 

Considering "qxl_gem_fini() was never called" did we have a leak?

> Signed-off-by: Christophe Fergeau 
> ---
>  drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index e642242..af685f1 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
>   mutex_init(&qdev->update_area_mutex);
>   mutex_init(&qdev->release_mutex);
>   mutex_init(&qdev->surf_evict_mutex);
> - INIT_LIST_HEAD(&qdev->gem.objects);
> + qxl_gem_init(qdev);
>  

Here qxl_gem_init returns a value that is always ignored, perhaps
would be better to return void from qxl_gem_init if it cannot
fails.

>   qdev->rom_base = pci_resource_start(pdev, 2);
>   qdev->rom_size = pci_resource_len(pdev, 2);
> @@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
>   qxl_ring_free(qdev->command_ring);
>   qxl_ring_free(qdev->cursor_ring);
>   qxl_ring_free(qdev->release_ring);
> + qxl_gem_fini(qdev);
>   qxl_bo_fini(qdev);
>   io_mapping_free(qdev->surface_mapping);
>   io_mapping_free(qdev->vram_mapping);

Frediano

[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Frediano Ziglio]

 = true;
> + }
> + if (client_head->width != c_rect->right - c_rect->left) {
> + client_head->width = c_rect->right - c_rect->left;
> + changed = true;
> + }
> + if (client_head->height != c_rect->bottom - c_rect->top) {
> + client_head->height = c_rect->bottom - c_rect->top;
> + changed = true;
> + }
> + if (client_head->surface_id != 0) {
> + client_head->surface_id = 0;
> + changed = true;
> + }
> + if (client_head->id != i) {
> + client_head->id = i;
> + changed = true;
> + }

quite similar code... I would write a macro but I'm a too macro fun :)
Would be something like this

if (client_head->id != i)
res = MONITORS_CONFIG_COPIED;
client_head->id = i;

make sense?

> + if (client_head->flags != 0) {
> + client_head->flags = 0;
> + changed = true;
> + }

why testing flags change if is always 0 ?

>   DRM_DEBUG_KMS("read %dx%d+%d+%d\n", client_head->width,
>   client_head->height,
> client_head->x, client_head->y);
>   }
> - return 0;
> + if (changed) {
> + return MONITORS_CONFIG_COPIED;
> + } else {
> + return MONITORS_CONFIG_UNCHANGED;
> + }

Here it would be just "return res;".

>  }
>  
>  static void qxl_update_offset_props(struct qxl_device *qdev)
> @@ -124,9 +160,18 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev)
>  {
>  
>   struct drm_device *dev = qdev->ddev;
> - while (qxl_display_copy_rom_client_monitors_config(qdev)) {
> + enum MonitorsConfigCopyStatus status;
> +
> + status = qxl_display_copy_rom_client_monitors_config(qdev);
> + while (status == MONITORS_CONFIG_BAD_CRC) {
>   qxl_io_log(qdev, "failed crc check for client_monitors_config,"
>" retrying\n");
> + status = qxl_display_copy_rom_client_monitors_config(qdev);
> + }

Usually I would write something like

for (;;) {
status = qxl_display_copy_rom_client_monitors_config(qdev);
if (status != MONITORS_CONFIG_BAD_CRC)
break;
    qxl_io_log(qdev, "failed crc check for client_monitors_config,"
 " retrying\n");
}

or

while ((status = qxl_display_copy_rom_client_monitors_config(qdev))
==  MONITORS_CONFIG_BAD_CRC) {
qxl_io_log(qdev, "failed crc check for client_monitors_config,"
 " retrying\n");
}

(just style and probably indentation is even wrong)

> + if (status == MONITORS_CONFIG_UNCHANGED) {
> + qxl_io_log(qdev, "config unchanged\n");
> + DRM_DEBUG("ignoring unchanged client monitors config");
> + return;
>   }
>  
>   drm_modeset_lock_all(dev);

Beside all that style paranoia/opinions:

Acked-by: Frediano Ziglio 

(feel free to take into account some or none of them).

Frediano

[PATCH] Do not loop on ERESTARTSYS using interruptible waits

[Frediano Ziglio]

This problem happens using KMS surfaces and QXL driver.
To easy reproduce use KDE Plasma (which use surfaces a lot) and assure
you are using KMS surfaces (QXL driver on Fedora/RedHat has a patch to
stop using them). Open some complex application like LibreOffice and
after a while your machine get stuck using 100% CPU on Xorg.
The problem occurs as creating new surfaces not interruptible wait
are used however instead of returning ERESTARTSYS back to userspace
you try to loop but wait routines always keep returning ERESTARTSYS
once the signal is marked.
On out of memory conditions TTM module try to move objects to system
memory and QXL assure surface is updated before the move.
The fix handle differently this case using no interruptible wait so
wait functions will wait instead of returning ERESTARTSYS.
Note the when the loop occurs driver will send a lot of update requests
causing more CPU usage on Qemu side too.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_cmd.c   | 12 +++-
 qxl/qxl_drv.h   |  2 +-
 qxl/qxl_ioctl.c |  2 +-
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index 9782364..bd5404e 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -317,14 +317,11 @@ static void wait_for_io_cmd(struct qxl_device *qdev, 
uint8_t val, long port)
 {
int ret;

-restart:
ret = wait_for_io_cmd_user(qdev, val, port, false);
-   if (ret == -ERESTARTSYS)
-   goto restart;
 }

 int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
-   const struct qxl_rect *area)
+   const struct qxl_rect *area, bool intr)
 {
int surface_id;
uint32_t surface_width, surface_height;
@@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct 
qxl_bo *surf,
mutex_lock(&qdev->update_area_mutex);
qdev->ram_header->update_area = *area;
qdev->ram_header->update_surface = surface_id;
-   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
+   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
mutex_unlock(&qdev->update_area_mutex);
return ret;
 }
@@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct 
qxl_bo *surf)
rect.right = surf->surf.width;
rect.top = 0;
rect.bottom = surf->surf.height;
-retry:
-   ret = qxl_io_update_area(qdev, surf, &rect);
-   if (ret == -ERESTARTSYS)
-   goto retry;
+   ret = qxl_io_update_area(qdev, surf, &rect, false);
return ret;
 }

diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_drv.h b/qxl/qxl_drv.h
index 7c6cafe..6745c44 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -462,7 +462,7 @@ void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t 
id);
 void qxl_io_notify_oom(struct qxl_device *qdev);

 int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
-  const struct qxl_rect *area);
+  const struct qxl_rect *area, bool intr);

 void qxl_io_reset(struct qxl_device *qdev);
 void qxl_io_monitors_config(struct qxl_device *qdev);
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index b110883..afd7297 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -348,7 +348,7 @@ static int qxl_update_area_ioctl(struct drm_device *dev, 
void *data,
goto out2;
if (!qobj->surface_id)
DRM_ERROR("got update area for surface with no id %d\n", 
update_area->handle);
-   ret = qxl_io_update_area(qdev, qobj, &area);
+   ret = qxl_io_update_area(qdev, qobj, &area, true);

 out2:
qxl_bo_unreserve(qobj);
-- 
2.1.0

[Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

[Frediano Ziglio]

> 
> Hey,
> 
> On Tue, May 19, 2015 at 05:54:54AM -0400, Frediano Ziglio wrote:
> > This problem happens using KMS surfaces and QXL driver.
> > To easy reproduce use KDE Plasma (which use surfaces a lot) and assure
> > you are using KMS surfaces (QXL driver on Fedora/RedHat has a patch to
> > stop using them). Open some complex application like LibreOffice and
> > after a while your machine get stuck using 100% CPU on Xorg.
> > The problem occurs as creating new surfaces not interruptible wait
> > are used however instead of returning ERESTARTSYS back to userspace
> > you try to loop but wait routines always keep returning ERESTARTSYS
> > once the signal is marked.
> > On out of memory conditions TTM module try to move objects to system
> > memory and QXL assure surface is updated before the move.
> > The fix handle differently this case using no interruptible wait so
> > wait functions will wait instead of returning ERESTARTSYS.
> > Note the when the loop occurs driver will send a lot of update requests
> > causing more CPU usage on Qemu side too.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  qxl/qxl_cmd.c   | 12 +++-
> >  qxl/qxl_drv.h   |  2 +-
> >  qxl/qxl_ioctl.c |  2 +-
> >  3 files changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
> > index 9782364..bd5404e 100644
> > --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> > +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> > @@ -317,14 +317,11 @@ static void wait_for_io_cmd(struct qxl_device *qdev,
> > uint8_t val, long port)
> >  {
> > int ret;
> >  
> > -restart:
> > ret = wait_for_io_cmd_user(qdev, val, port, false);
> > -   if (ret == -ERESTARTSYS)
> > -   goto restart;
> 
> I think this one is not directly related to the fix, but can be removed
> because wait_for_io_cmd_user(qdev, val, port, false); will call
> wait_event_timeout() which cannot return ERESTARTSYS? Or was this loop
> causing issues too?
> 

Yes, but it has the same issue. Try till ERESTARTSYS are gone.
Currently perhaps not broken but prone to have same issue.

> >  }
> >  
> >  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
> > -   const struct qxl_rect *area)
> > +   const struct qxl_rect *area, bool intr)
> >  {
> > int surface_id;
> > uint32_t surface_width, surface_height;
> > @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct
> > qxl_bo *surf,
> > mutex_lock(&qdev->update_area_mutex);
> > qdev->ram_header->update_area = *area;
> > qdev->ram_header->update_surface = surface_id;
> > -   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
> > +   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
> > mutex_unlock(&qdev->update_area_mutex);
> > return ret;
> >  }
> > @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct
> > qxl_bo *surf)
> > rect.right = surf->surf.width;
> > rect.top = 0;
> > rect.bottom = surf->surf.height;
> > -retry:
> > -   ret = qxl_io_update_area(qdev, surf, &rect);
> > -   if (ret == -ERESTARTSYS)
> > -   goto retry;
> > +   ret = qxl_io_update_area(qdev, surf, &rect, false);
> 
> My understanding is that the fix is this hunk? If so, this could be made
> more obvious with an intermediate commit adding the 'bool intr' arg to
> qxl_io_update_area and only calling it with 'true' in the appropriate
> places.
> This code path is only triggered from qxl_surface_evict() which I assume
> is not necessarily easily interruptible, so this change makes sense to
> me. However it would be much better to get a review from Dave Airlie ;)
> 
> Christophe
> 

Are you asking if just removing the loop would fix the issue?
So you are proposing a first patch that add the argument always passing true 
and another that change some calls to false? It make sense but still the loop 
should be removed.

Frediano

[PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

Frediano Ziglio (11):
  Do not cause spice-server to clean our objects
  Do not leak memory if qxl_release_list_add fails
  Fix print statement not using uninitialized variable
  Avoid double free on error
  Handle all errors in qxl_surface_evict
  Fix return for qxl_release_alloc
  Handle correctly failures in qxl_alloc_relase_reserved
  Remove format string errors
  Move main reference counter to GEM object instead of TTM ones
  Simplify cleaning qxl processing command
  Propagate correctly errors from qxlhw_handle_to_bo

 qxl/qxl_cmd.c | 11 ++-
 qxl/qxl_display.c |  2 +-
 qxl/qxl_drv.h |  2 +-
 qxl/qxl_gem.c | 10 --
 qxl/qxl_ioctl.c   | 46 +-
 qxl/qxl_object.c  | 11 ---
 qxl/qxl_release.c | 13 +
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH 01/11] Do not cause spice-server to clean our objects

[Frediano Ziglio]

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,

cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
cmd->type = QXL_SURFACE_CMD_CREATE;
+   cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
cmd->u.surface_create.format = surf->surf.format;
cmd->u.surface_create.width = surf->surf.width;
cmd->u.surface_create.height = surf->surf.height;
-- 
2.1.0

[PATCH 02/11] Do not leak memory if qxl_release_list_add fails

[Frediano Ziglio]

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
-   if (ret)
+   if (ret) {
+   drm_gem_object_unreference_unlocked(gobj);
return NULL;
+   }

return qobj;
 }
-- 
2.1.0

[PATCH 03/11] Fix print statement not using uninitialized variable

[Frediano Ziglio]

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
/* add the bos to the list of bos to validate -
   need to validate first then process relocs? */
if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type 
!= QXL_RELOC_TYPE_SURF) {
-   DRM_DEBUG("unknown reloc type %d\n", 
reloc_info[i].type);
+   DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);

ret = -EINVAL;
goto out_free_bos;
-- 
2.1.0

[PATCH 04/11] Avoid double free on error

[Frediano Ziglio]

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxlhw_handle_to_bo(qdev, file_priv,
   reloc.src_handle, release);
if (!reloc_info[i].src_bo) {
-   if (reloc_info[i].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base);
ret = -EINVAL;
goto out_free_bos;
}
-- 
2.1.0

[PATCH 05/11] Handle all errors in qxl_surface_evict

[Frediano Ziglio]

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_cmd.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal
int ret;

ret = qxl_bo_reserve(surf, false);
-   if (ret == -EBUSY)
-   return -EBUSY;
+   if (ret)
+   return ret;

if (stall)
mutex_unlock(&qdev->surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal

if (stall)
mutex_lock(&qdev->surf_evict_mutex);
-   if (ret == -EBUSY) {
+   if (ret) {
qxl_bo_unreserve(surf);
-   return -EBUSY;
+   return ret;
}

qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

[PATCH 06/11] Fix return for qxl_release_alloc

[Frediano Ziglio]

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
.wait = qxl_fence_wait,
 };

-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
  struct qxl_release **ret)
 {
-- 
2.1.0

[PATCH 07/11] Handle correctly failures in qxl_alloc_relase_reserved

[Frediano Ziglio]

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_release.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,
ret = qxl_release_bo_alloc(qdev, 
&qdev->current_release_bo[cur_idx]);
if (ret) {
mutex_unlock(&qdev->release_mutex);
+   qxl_release_free(qdev, *release);
return ret;
}
}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,

mutex_unlock(&qdev->release_mutex);

-   qxl_release_list_add(*release, bo);
+   ret = qxl_release_list_add(*release, bo);
+   qxl_bo_unref(&bo);
+   if (ret) {
+   qxl_release_free(qdev, *release);
+   return ret;
+   }

info = qxl_release_map(qdev, *release);
info->id = idr_ret;
qxl_release_unmap(qdev, *release, info);

-   qxl_bo_unref(&bo);
return ret;
 }

-- 
2.1.0

[PATCH 08/11] Remove format string errors

[Frediano Ziglio]

Enable format string checks for qxl_io_log and remove resulting warnings
which could lead to memory errors on different platform or just printing
wrong information.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_cmd.c | 2 +-
 qxl/qxl_display.c | 2 +-
 qxl/qxl_drv.h | 2 +-
 qxl/qxl_release.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index b18f84c..edc1eec 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
}
}

-   QXL_INFO(qdev, "%s: %lld\n", __func__, i);
+   QXL_INFO(qdev, "%s: %d\n", __func__, i);

return i;
 }
diff --git a/qxl/qxl_display.c b/qxl/qxl_display.c
index 4a0a8b2..a8dbb3e 100644
--- a/qxl/qxl_display.c
+++ b/qxl/qxl_display.c
@@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
  sizeof(qdev->rom->client_monitors_config));
if (crc != qdev->rom->client_monitors_config_crc) {
-   qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
+   qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
   sizeof(qdev->rom->client_monitors_config),
   qdev->rom->client_monitors_config_crc);
return 1;
diff --git a/qxl/qxl_drv.h b/qxl/qxl_drv.h
index 6745c44..62ef8be 100644
--- a/qxl/qxl_drv.h
+++ b/qxl/qxl_drv.h
@@ -328,7 +328,7 @@ struct qxl_device {
 };

 /* forward declaration for QXL_INFO_IO */
-void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
+__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);

 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index 00604ed..b66ec33 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
return handle;
}
*ret = release;
-   QXL_INFO(qdev, "allocated release %lld\n", handle);
+   QXL_INFO(qdev, "allocated release %d\n", handle);
release->id = handle;
return handle;
 }
-- 
2.1.0

[PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_gem.c| 10 --
 qxl/qxl_object.c | 11 ---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/qxl/qxl_gem.c b/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/qxl/qxl_gem.c
+++ b/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+   struct qxl_device *qdev;
+   struct ttm_buffer_object *tbo;

-   if (qobj)
-   qxl_bo_unref(&qobj);
+   qdev = (struct qxl_device *)gobj->dev->dev_private;
+
+   qxl_surface_evict(qdev, qobj, false);
+
+   tbo = &qobj->tbo;
+   ttm_bo_unref(&tbo);
 }

 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/qxl/qxl_object.c b/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/qxl/qxl_object.c
+++ b/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,

 void qxl_bo_unref(struct qxl_bo **bo)
 {
-   struct ttm_buffer_object *tbo;
-
if ((*bo) == NULL)
return;
-   tbo = &((*bo)->tbo);
-   ttm_bo_unref(&tbo);
-   if (tbo == NULL)
-   *bo = NULL;
+
+   drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+   *bo = NULL;
 }

 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-   ttm_bo_reference(&bo->tbo);
+   drm_gem_object_reference(&bo->gem_base);
return bo;
 }

-- 
2.1.0

[PATCH 10/11] Simplify cleaning qxl processing command

[Frediano Ziglio]

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_ioctl.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
-   if (ret) {
-   drm_gem_object_unreference_unlocked(gobj);
+   drm_gem_object_unreference_unlocked(gobj);
+   if (ret)
return NULL;
-   }

return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
struct qxl_release *release;
struct qxl_bo *cmd_bo;
void *fb_cmd;
-   int i, j, ret, num_relocs;
+   int i, ret, num_relocs;
int unwritten;

switch (cmd->type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxl_release_fence_buffer_objects(release);

 out_free_bos:
-   for (j = 0; j < num_relocs; j++) {
-   if (reloc_info[j].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
-   if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
-   }
 out_free_release:
if (ret)
qxl_release_free(qdev, release);
-- 
2.1.0

[PATCH 11/11] Propagate correctly errors from qxlhw_handle_to_bo

[Frediano Ziglio]

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio 
---
 qxl/qxl_ioctl.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct 
qxl_reloc_info *info)
 }

 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-struct drm_file *file_priv, uint64_t 
handle,
-struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+ struct drm_file *file_priv, uint64_t handle,
+ struct qxl_release *release, struct qxl_bo 
**qbo_p)
 {
struct drm_gem_object *gobj;
struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct 
qxl_device *qdev,

gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
if (!gobj)
-   return NULL;
+   return -EINVAL;

qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
drm_gem_object_unreference_unlocked(gobj);
if (ret)
-   return NULL;
+   return ret;

-   return qobj;
+   *qbo_p = qobj;
+   return 0;
 }

 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
reloc_info[i].type = reloc.reloc_type;

if (reloc.dst_handle) {
-   reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, 
file_priv,
- 
reloc.dst_handle, release);
-   if (!reloc_info[i].dst_bo) {
-   ret = -EINVAL;
-   reloc_info[i].src_bo = NULL;
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.dst_handle, release,
+&reloc_info[i].dst_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].dst_offset = reloc.dst_offset;
} else {
reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
num_relocs++;

/* reserve and validate the reloc dst bo */
-   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 
0) {
-   reloc_info[i].src_bo =
-   qxlhw_handle_to_bo(qdev, file_priv,
-  reloc.src_handle, release);
-   if (!reloc_info[i].src_bo) {
-   ret = -EINVAL;
+   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.src_handle, release,
+&reloc_info[i].src_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].src_offset = reloc.src_offset;
} else {
reloc_info[i].src_bo = NULL;
-- 
2.1.0

[PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

> 
> On Wed, May 27, 2015 at 8:47 AM, Josh Boyer 
> wrote:
> > On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio 
> > wrote:
> >> This set of patches mainly contains fix for some memory issues
> >> using quite aggressively surfaces and other minor problems like
> >> images going black after a while.
> >>
> >> Frediano Ziglio (11):
> >>   Do not cause spice-server to clean our objects
> >>   Do not leak memory if qxl_release_list_add fails
> >>   Fix print statement not using uninitialized variable
> >>   Avoid double free on error
> >>   Handle all errors in qxl_surface_evict
> >>   Fix return for qxl_release_alloc
> >>   Handle correctly failures in qxl_alloc_relase_reserved
> >>   Remove format string errors
> >>   Move main reference counter to GEM object instead of TTM ones
> >>   Simplify cleaning qxl processing command
> >>   Propagate correctly errors from qxlhw_handle_to_bo
> >>
> >>  qxl/qxl_cmd.c | 11 ++-
> >>  qxl/qxl_display.c |  2 +-
> >>  qxl/qxl_drv.h |  2 +-
> >>  qxl/qxl_gem.c | 10 --
> >>  qxl/qxl_ioctl.c   | 46 +-
> >>  qxl/qxl_object.c  | 11 ---
> >>  qxl/qxl_release.c | 13 +
> >>  7 files changed, 46 insertions(+), 49 deletions(-)
> >
> > The strip level on these patches is rather odd.  Normally one would
> > see a strip level of 1 at the top of the kernel dir.  E.g.
> >
> > drivers/gpu/drm/qxl/qxl_gem.c
> >
> > in the diffstat, etc.
> 
> (Sorry for the double reply.)
> 
> Also, are any of these commits something that should be queued for
> stable kernel releases?  There are a handful that look like they
> should be to me.
> 
> josh
> 

Hi,
  no problem for double reply.

I was using a different repository with only QXL driver. I tested and all 
patches apply and compile perfectly even with Linus master branch.

About which patches should be applied surely (attempting to put a priority)
- "Move main reference counter to GEM object instead of TTM ones" this can 
causes memory corruption even not wanting to;
- "Avoid double free on error" this can be cause leaks in kernel if user space 
wants, mitigated by the fact that usually DRM inodes are owned by root;
- "Handle all errors in qxl_surface_evict" could cause corruption too, not 
really probable but taking into account that Xorg implementation use a lot 
signals is not so impossible;
- "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory 
if qxl_release_list_add fails" just cause leaks on situation where memory is 
already REALLY low, can be omitted;
- "Fix print statement not using uninitialized variable", "Remove format string 
errors" should just print garbage and debugging is disabled by default, not 
necessary.

Frediano

[Spice-devel] [PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

> 
> > I was using a different repository with only QXL driver. I tested and all
> > patches apply and compile perfectly even with Linus master branch.
> 
> Lets only post patches people can apply, it makes it harder to figure
> out stuff. I'll take a look at the patches, but it would be good to
> get them resent base on drm-next.
> 

I'll do.

> also indicating in each patch what is a right now fix and what isn't.
> 

What do you mean by right fix or not ?
In the cover specify the sort of information I give to Josh ?

> Dave.
> 

Frediano

[Spice-devel] [PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

> On 27 May 2015 at 20:04, Frediano Ziglio  wrote:
> > qxl_bo structure has two reference counters, one in the GEM object and
> > another in the TTM object. The GEM object keep a counter to the TTM object
> > so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
> > decremented. The qxl object is fully freed (both GEM and TTM part are
> > cleaned)
> > when the TTM counter reach zero.
> > One issue was that surface idr structure has no owning on qxl_bo objects
> > however
> > it contains a pointer to qxl_bo object. This caused some nasty race
> > condition
> > for instance qxl_bo object was reaped even after counter was already zero.
> > This patch fix these races moving main counter (the one used by
> > qxl_bo_(un)ref)
> > to GEM object which cleanup routine (qxl_gem_object_free) remove the idr
> > pointer
> > (using qxl_surface_evict) when the counters are still valid.
> 
> Uggh, but yes, not sure I like this fix for the problem, but if it works,
> 
> Reviewed-by: Dave Airlie 
> 

Well, the patch does not surely looks very clear but I can assure the problems 
it fixes are much less clear to understand.
Having a pointer to a object the is going to be deleted whenever another thread 
decide to causes difficult races. I tried to avoid this kind of change and fix 
the races instead but was a nightmare.
My first experimental patch added an additional counter on top of GEM and TTM 
one as the main counter but at the end was much more complicated and result was 
similar to move the main counter to GEM.
Mainly external references (from userspace and kernel) are pointers to GEM. 
Pointers to TTM are from memory mapped files. Deleting the spice id after GEM 
object has no references assure the not owning reference from spice id still 
refer to a valid object. As user can't retrieve a pointer from a mapping (at 
most can remap it) so there are no risks counter to GEM is incremented again.

Frediano

[patch 4/4 v2] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()

[Frediano Ziglio]

> 
> The size calculation can overflow.  I don't know if this leads to
> memory corruption, but it causes a static checker warning.
> 
> Signed-off-by: Dan Carpenter 
> ---
> v2: I don't know think the size is capped anywhere.  In my first version
> of this patch, I introduced a divide by zero bug.
> 

Beside the second sentence I would ack

Frediano

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index b2db482..49b3158 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -396,12 +396,14 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev,
> void *data,
>   struct qxl_bo *qobj;
>   int handle;
>   int ret;
> - int size, actual_stride;
> + u64 size, actual_stride;
>   struct qxl_surface surf;
>  
>   /* work out size allocate bo with handle */
>   actual_stride = param->stride < 0 ? -param->stride : param->stride;
>   size = actual_stride * param->height + actual_stride;
> + if (size > INT_MAX)
> + return -EINVAL;
>  
>   surf.format = param->format;
>   surf.width = param->width;
> 

[PATCH] drm/qxl: use to_qxl_bo macro

[Frediano Ziglio]

Instead of using container_of directly use to_qxl_bo macro.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_object.c | 2 +-
 drivers/gpu/drm/qxl/qxl_ttm.c| 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b28370e..5e1d789 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -32,7 +32,7 @@ static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
struct qxl_bo *bo;
struct qxl_device *qdev;

-   bo = container_of(tbo, struct qxl_bo, tbo);
+   bo = to_qxl_bo(tbo);
qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;

qxl_surface_evict(qdev, bo, false);
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 0cbc4c9..9534127 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -201,7 +201,7 @@ static void qxl_evict_flags(struct ttm_buffer_object *bo,
placement->num_busy_placement = 1;
return;
}
-   qbo = container_of(bo, struct qxl_bo, tbo);
+   qbo = to_qxl_bo(bo);
qxl_ttm_placement_from_domain(qbo, QXL_GEM_DOMAIN_CPU, false);
*placement = qbo->placement;
 }
@@ -365,7 +365,7 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo,

if (!qxl_ttm_bo_is_qxl_bo(bo))
return;
-   qbo = container_of(bo, struct qxl_bo, tbo);
+   qbo = to_qxl_bo(bo);
qdev = qbo->gem_base.dev->dev_private;

if (bo->mem.mem_type == TTM_PL_PRIV0 && qbo->surface_id)
-- 
2.4.3

[PATCH 0/2] Dependency locks fixes for QXL driver

[Frediano Ziglio]

These two patches fixes two lock dependencies issues.

Frediano Ziglio (2):
  drm/qxl: use to_qxl_bo macro
  drm/qxl: avoid dependency lock

 drivers/gpu/drm/qxl/qxl_object.c  | 2 +-
 drivers/gpu/drm/qxl/qxl_release.c | 4 +---
 drivers/gpu/drm/qxl/qxl_ttm.c | 4 ++--
 3 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.4.3

[PATCH 1/2] drm/qxl: use to_qxl_bo macro

[Frediano Ziglio]

Instead of using container_of directly use to_qxl_bo macro.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_object.c | 2 +-
 drivers/gpu/drm/qxl/qxl_ttm.c| 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b28370e..5e1d789 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -32,7 +32,7 @@ static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
struct qxl_bo *bo;
struct qxl_device *qdev;

-   bo = container_of(tbo, struct qxl_bo, tbo);
+   bo = to_qxl_bo(tbo);
qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;

qxl_surface_evict(qdev, bo, false);
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 0cbc4c9..9534127 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -201,7 +201,7 @@ static void qxl_evict_flags(struct ttm_buffer_object *bo,
placement->num_busy_placement = 1;
return;
}
-   qbo = container_of(bo, struct qxl_bo, tbo);
+   qbo = to_qxl_bo(bo);
qxl_ttm_placement_from_domain(qbo, QXL_GEM_DOMAIN_CPU, false);
*placement = qbo->placement;
 }
@@ -365,7 +365,7 @@ static void qxl_bo_move_notify(struct ttm_buffer_object *bo,

if (!qxl_ttm_bo_is_qxl_bo(bo))
return;
-   qbo = container_of(bo, struct qxl_bo, tbo);
+   qbo = to_qxl_bo(bo);
qdev = qbo->gem_base.dev->dev_private;

if (bo->mem.mem_type == TTM_PL_PRIV0 && qbo->surface_id)
-- 
2.4.3

[PATCH 2/2] drm/qxl: avoid dependency lock

[Frediano Ziglio]

qxl_bo_unref calls drm_gem_object_unreference_unlocked which
locks dev->struct_mutex. However this lock could be already
locked if the call came from qxl_gem_object_free.
As we don't need to call qxl_bo_ref/qxl_bo_unref cause
qxl_release_list_add will hold a reference by itself avoid
to call them and the possible deadlock.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_release.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c 
b/drivers/gpu/drm/qxl/qxl_release.c
index b66ec33..4efa8e2 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -307,7 +307,7 @@ int qxl_alloc_surface_release_reserved(struct qxl_device 
*qdev,
idr_ret = qxl_release_alloc(qdev, QXL_RELEASE_SURFACE_CMD, 
release);
if (idr_ret < 0)
return idr_ret;
-   bo = qxl_bo_ref(to_qxl_bo(entry->tv.bo));
+   bo = to_qxl_bo(entry->tv.bo);

(*release)->release_offset = create_rel->release_offset + 64;

@@ -316,8 +316,6 @@ int qxl_alloc_surface_release_reserved(struct qxl_device 
*qdev,
info = qxl_release_map(qdev, *release);
info->id = idr_ret;
qxl_release_unmap(qdev, *release, info);
-
-   qxl_bo_unref(&bo);
return 0;
}

-- 
2.4.3

[PATCH v2 0/2] Dependency locks fixes for QXL driver

[Frediano Ziglio]

These two patches fixes two lock dependencies issues.

Changes from v1: Sent a wrong patch.

Frediano Ziglio (2):
  drm/qxl: avoid buffer reservation in qxl_crtc_page_flip
  drm/qxl: avoid dependency lock

 drivers/gpu/drm/qxl/qxl_display.c | 10 +-
 drivers/gpu/drm/qxl/qxl_release.c |  4 +---
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.4.3

[PATCH v2 1/2] drm/qxl: avoid buffer reservation in qxl_crtc_page_flip

[Frediano Ziglio]

This avoid a dependency lock error.
According to https://lwn.net/Articles/548909/ users of WW mutex API
should avoid using different context.
When a buffer is reserved with qxl_bo_reserve a ww_mutex_lock without
context is used. However during qxl_draw_dirty_fb different locks
with specific context are used.
This is detected during a machine booting with a debug kernel with lock
dependency checking enabled.
Like many other function in this file to avoid this problem object
pinning is used. Once the object is pinned is not necessary to keep
the lock so it can be released avoiding the locking problem.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_display.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index a8dbb3e..656752d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -241,6 +241,10 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc,
ret = qxl_bo_reserve(bo, false);
if (ret)
return ret;
+   ret = qxl_bo_pin(bo, bo->type, NULL);
+   qxl_bo_unreserve(bo);
+   if (ret)
+   return ret;

qxl_draw_dirty_fb(qdev, qfb_src, bo, 0, 0,
  &norect, one_clip_rect, inc);
@@ -254,7 +258,11 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc,
}
drm_vblank_put(dev, qcrtc->index);

-   qxl_bo_unreserve(bo);
+   ret = qxl_bo_reserve(bo, false);
+   if (!ret) {
+   qxl_bo_unpin(bo);
+   qxl_bo_unreserve(bo);
+   }

return 0;
 }
-- 
2.4.3

[PATCH v2 2/2] drm/qxl: avoid dependency lock

[Frediano Ziglio]

qxl_bo_unref calls drm_gem_object_unreference_unlocked which
locks dev->struct_mutex. However this lock could be already
locked if the call came from qxl_gem_object_free.
As we don't need to call qxl_bo_ref/qxl_bo_unref cause
qxl_release_list_add will hold a reference by itself avoid
to call them and the possible deadlock.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_release.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c 
b/drivers/gpu/drm/qxl/qxl_release.c
index b66ec33..4efa8e2 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -307,7 +307,7 @@ int qxl_alloc_surface_release_reserved(struct qxl_device 
*qdev,
idr_ret = qxl_release_alloc(qdev, QXL_RELEASE_SURFACE_CMD, 
release);
if (idr_ret < 0)
return idr_ret;
-   bo = qxl_bo_ref(to_qxl_bo(entry->tv.bo));
+   bo = to_qxl_bo(entry->tv.bo);

(*release)->release_offset = create_rel->release_offset + 64;

@@ -316,8 +316,6 @@ int qxl_alloc_surface_release_reserved(struct qxl_device 
*qdev,
info = qxl_release_map(qdev, *release);
info->id = idr_ret;
qxl_release_unmap(qdev, *release, info);
-
-   qxl_bo_unref(&bo);
return 0;
}

-- 
2.4.3

Re: [Spice-devel] [PATCH 1/3] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Frediano Ziglio]

> qxl surfaces (used for framebuffers and gem objects) can live in both
> VRAM and PRIV ttm domains.  Update placement setup to include both.  Put
> PRIV first in the list so it is preferred, so VRAM will have more room
> for objects which must be allocated there.
> 
> Signed-off-by: Gerd Hoffmann 

I remember these kind of changes in the past made migration
fails. I proposed similar patches years ago and they were rejected
for these reasons. Why now they are safe?

Looks like we are improving QXL, so that means we are actively working
on it. Should we not then thinking about moving feature in the proper
places (like spice-server for atomic mode setting instead of implementin
work around) ??

> ---
>  drivers/gpu/drm/qxl/qxl_object.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> b/drivers/gpu/drm/qxl/qxl_object.c
> index 91f3bbc73e..f7f9f4f4fe 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -58,10 +58,10 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo,
> u32 domain, bool pinned)
>  
>   qbo->placement.placement = qbo->placements;
>   qbo->placement.busy_placement = qbo->placements;
> - if (domain == QXL_GEM_DOMAIN_VRAM)
> - qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
> TTM_PL_FLAG_VRAM |
> pflag;
>   if (domain == QXL_GEM_DOMAIN_SURFACE)
>   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
> TTM_PL_FLAG_PRIV |
>   pflag;
> + if (domain == QXL_GEM_DOMAIN_SURFACE || domain == QXL_GEM_DOMAIN_VRAM)
> + qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
> TTM_PL_FLAG_VRAM |
> pflag;
>   if (domain == QXL_GEM_DOMAIN_CPU)
>   qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
> TTM_PL_FLAG_SYSTEM |
>   pflag;
>   if (!c)

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

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> Just use qxl_num_crtc directly everywhere instead of using
> qdev->monitors_config->max_allowed.  Drops pointless indirection
> and also is less confusing.
> 

To me is MORE confusing, why comparing number of something with
another number? Previously code was comparing number of monitors
with number of monitors, not number of CRTs with number of
monitors.

Why we are optimizing code that, as yours same saying, we are
not much actively improving?

> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ce0b9c40fc..6437369a31 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -80,10 +80,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   DRM_DEBUG_KMS("no client monitors configured\n");
>   return status;
>   }
> - if (num_monitors > qdev->monitors_config->max_allowed) {
> + if (num_monitors > qxl_num_crtc) {
>   DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
> %d\n",
> -   qdev->monitors_config->max_allowed, num_monitors);
> - num_monitors = qdev->monitors_config->max_allowed;
> +   qxl_num_crtc, num_monitors);
> + num_monitors = qxl_num_crtc;
>   } else {
>   num_monitors = qdev->rom->client_monitors_config.count;
>   }
> @@ -96,8 +96,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   return status;
>   }
>   /* we copy max from the client but it isn't used */
> - qdev->client_monitors_config->max_allowed =
> - qdev->monitors_config->max_allowed;
> + qdev->client_monitors_config->max_allowed = qxl_num_crtc;
>   for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
>   struct qxl_urect *c_rect =
>   &qdev->rom->client_monitors_config.heads[i];
> @@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct
> drm_connector *connector,
>  
>   if (!qdev->monitors_config)
>   return 0;
> - if (h >= qdev->monitors_config->max_allowed)
> + if (h >= qxl_num_crtc)
>   return 0;
>   if (!qdev->client_monitors_config)
>   return 0;
> @@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>   return;
>   }
>  
> - if (!qdev->monitors_config ||
> - qdev->monitors_config->max_allowed <= i)
> + if (!qdev->monitors_config || qxl_num_crtc <= i)
>   return;
>  
>   head.id = i;
> @@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>   if (oldcount != qdev->monitors_config->count)
>   DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
> oldcount, qdev->monitors_config->count,
> -   qdev->monitors_config->max_allowed);
> +   qxl_num_crtc);
>  
>   qdev->monitors_config->heads[i] = head;
> + qdev->monitors_config->max_allowed = qxl_num_crtc;
>   qxl_send_monitors_config(qdev);
>  }
>  
> @@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  {
>   int ret;
>   struct drm_gem_object *gobj;
> - int max_allowed = qxl_num_crtc;
>   int monitors_config_size = sizeof(struct qxl_monitors_config) +
> - max_allowed * sizeof(struct qxl_head);
> + qxl_num_crtc * sizeof(struct qxl_head);
>  
>   ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
>   QXL_GEM_DOMAIN_VRAM,
> @@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>   qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
>  
>   memset(qdev->monitors_config, 0, monitors_config_size);
> - qdev->monitors_config->max_allowed = max_allowed;
>   return 0;
>  }
>  

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

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > 
> > > Just use qxl_num_crtc directly everywhere instead of using
> > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > and also is less confusing.
> > > 
> > 
> > To me is MORE confusing, why comparing number of something with
> > another number? Previously code was comparing number of monitors
> > with number of monitors, not number of CRTs with number of
> > monitors.
> 
> Yes, spice/qxl and drm/kms use slightly different terminology.
> 
> drm crtc == qxl monitor.
> drm framebuffer == qxl surface.
> 
> You need to know that anyway when looking at the qxl ksm code.  We
> have function names like qxl_crtc_update_monitors_config().  I fail
> to see why that is a problem ...
> 
> cheers,
>   Gerd
> 
> 

I don't see any problem too but you are explaining to me
why your rationale "and also is less confusing" does not
stand.

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

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > > and also is less confusing.
> > > > > 
> > > > 
> > > > To me is MORE confusing, why comparing number of something with
> > > > another number? Previously code was comparing number of monitors
> > > > with number of monitors, not number of CRTs with number of
> > > > monitors.
> > > 
> > > Yes, spice/qxl and drm/kms use slightly different terminology.
> > > 
> > > drm crtc == qxl monitor.
> > > drm framebuffer == qxl surface.
> > > 
> > > You need to know that anyway when looking at the qxl ksm code.  We
> > > have function names like qxl_crtc_update_monitors_config().  I fail
> > > to see why that is a problem ...
> > > 
> > > cheers,
> > >   Gerd
> > 
> > I don't see any problem too but you are explaining to me
> > why your rationale "and also is less confusing" does not
> > stand.
> 
> Well, it's less confusing because it takes away an indirection (not
> because of the naming).
> 

It does not confuse me.

> qdev->monitors_config->max_allowed is effectively set by a module
> parameter.  So using the module parameter variable qxl_num_crtc
> directly is better IMO.  The kernel doesn't need to dereference pointers
> each time it needs the value, and when reading the code you don't have
> to trace where and why qdev->monitors_config->max_allowed is set.
> 

That should go to the commit message!
With that the patch is fine for me.

Maybe there's no much point on reusing the same structure used
inside QXLRom/QXLRam but this is OT for this patch.

> cheers,
>   Gerd
> 
> 

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

Re: [Spice-devel] [PATCH 1/3] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 05:55:58AM -0500, Frediano Ziglio wrote:
> > 
> > > qxl surfaces (used for framebuffers and gem objects) can live in both
> > > VRAM and PRIV ttm domains.  Update placement setup to include both.  Put
> > > PRIV first in the list so it is preferred, so VRAM will have more room
> > > for objects which must be allocated there.
> > > 
> > > Signed-off-by: Gerd Hoffmann 
> > 
> > I remember these kind of changes in the past made migration
> > fails. I proposed similar patches years ago and they were rejected
> > for these reasons.
> 
> Pointer?
> 

I think is this:
https://patchwork.kernel.org/patch/7374931/

I think all started with Windows where we have:
https://gitlab.freedesktop.org/spice/win32/qxl-wddm-dod/commit/0214d5ceda3f0da94de3813fc902150d497c6b26
https://gitlab.freedesktop.org/spice/win32/qxl-wddm-dod/commit/54a719e14f1204143da2c64f8a2aaee4fe5cd7d6

> > Why now they are safe?
> 
> Well, you have to be careful what object you are allocating.  Surfaces
> can be in both PRIV and VRAM.  Everything else (qxl commands, monitors
> config, ...) must be in VRAM.
> 
> > Looks like we are improving QXL, so that means we are actively working
> > on it.
> 
> Well, I'm just trying make qxl behave better with wayland.
> 

As far as I remember the Linux kernel driver simulates the frame buffer
swap with drawings which cause more memory copies.
Not also sure if this workaround make the server consume more network
bandwidth.
Are we supporting multiple monitors for Wayland?

> > Should we not then thinking about moving feature in the proper
> > places (like spice-server for atomic mode setting instead of implementin
> > work around) ??
> 
> Main advantage is that it doesn't need qxl device changes, so it works
> on old hosts too.  But, yes, we can consider to also modernize spice
> protocol and qxl device.
> 
> cheers,
>   Gerd
> 
> 

On the other hand we faced some bugs due these workarounds so we end up
with a solution that is less optimal than before and potentially
with more bugs to fix.
At the end we sell to customer a worst product.

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

Re: [Spice-devel] [PATCH AUTOSEL 6.5 2/6] drm/qxl: prevent memory leak

[Frediano Ziglio]

Il giorno mer 15 nov 2023 alle ore 06:57 Sasha Levin
 ha scritto:
>
> From: Zongmin Zhou 
>
> [ Upstream commit 0e8b9f258baed25f1c5672613699247c76b007b5 ]
>
> The allocated memory for qdev->dumb_heads should be released
> in qxl_destroy_monitors_object before qxl suspend.
> otherwise,qxl_create_monitors_object will be called to

Minor, typo: otherwise -> Otherwise.

> reallocate memory for qdev->dumb_heads after qxl resume,
> it will cause memory leak.
>
> Signed-off-by: Zongmin Zhou 
> Link: 
> https://lore.kernel.org/r/20230801025309.4049813-1-zhouzong...@kylinos.cn
> Reviewed-by: Dave Airlie 
> Signed-off-by: Maxime Ripard 
> Signed-off-by: Sasha Levin 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 6492a70e3c396..404b0483bb7cb 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1229,6 +1229,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
> if (!qdev->monitors_config_bo)
> return 0;
>
> +   kfree(qdev->dumb_heads);
> +   qdev->dumb_heads = NULL;
> +
> qdev->monitors_config = NULL;
> qdev->ram_header->monitors_config = 0;
>

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

[Frediano Ziglio]

> 
> From: Dave Airlie 
> 
> QXL hw can't change primary surfaces easily, the server sends a msg
> and the client flickers a lo when it does. The old pre-atomic

typo: lo -> lot

> page flip code endeavoured to workaround this with a copy operation.
> 

not clear. Do you mean a memcpy like operation or a DRAW_COPY ?

> This worked by another accident of how the qxl virtual gpu is designed,
> it does lazy operation evaluation on the host, so this operation
> wouldn't generally trash the memory, and result in correct display.
> 
> This tries to work out when the commit is likely just a pageflip
> and avoid touching the primary surface, this might go wrong at
> some point but I believe it's the same level as wrong as the old code
> base.
> 

This sentence contains lot of hopes :-)

> Signed-off-by: Dave Airlie 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index afbf50d..3b702d1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   struct qxl_framebuffer *qfb_old;
>   struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
>   struct qxl_bo *bo_old;
> + bool update_primary = true;
>   struct drm_clip_rect norect = {
>   .x1 = 0,
>   .y1 = 0,
> @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   if (bo == bo_old)
>   return;
>  
> + if (bo && bo_old &&
> + plane->state->crtc == old_state->crtc &&
> + plane->state->crtc_w == old_state->crtc_w &&
> + plane->state->crtc_h == old_state->crtc_h &&
> + plane->state->src_x == old_state->src_x &&
> + plane->state->src_y == old_state->src_y &&
> + plane->state->src_w == old_state->src_w &&
> + plane->state->src_h == old_state->src_h &&
> + plane->state->rotation == old_state->rotation &&
> + plane->state->zpos == old_state->zpos)
> + /* this is likely a pageflip */
> + update_primary = false;
> +
>   if (bo_old && bo_old->is_primary) {
> - qxl_io_destroy_primary(qdev);
> + if (update_primary)
> + qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
>   }
>  
>   if (!bo->is_primary) {
> - qxl_io_create_primary(qdev, 0, bo);
> + if (update_primary)
> + qxl_io_create_primary(qdev, 0, bo);
>   bo->is_primary = true;
>   }
> +
>   qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, &norect, 1, 1);
>  }
>  

What is actually supposed to do this function?
It looks weird. Looks like it's mixing drawing and surfaces together.
I don't see the reason to issue a DRAW_COPY (qxl_draw_dirty_fb) if
you just created the primary surface with data.
Also there's a big difference with and without this patch.
You are not changing the primary surface so the frame buffer (exactly
where spice-server draws the commands in QXL memory and where a screen
capture is supposed to find updated pixel after an update command)
still refers to bo_old object. However I don't see a related change in
the life management of bo_old. So or is going to be broken or was broken
before.

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

Re: [Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

[Frediano Ziglio]

> 
> From: Dave Airlie 
> 
> QXL hw can't change primary surfaces easily, the server sends a msg
> and the client flickers a lo when it does. The old pre-atomic
> page flip code endeavoured to workaround this with a copy operation.
> 
> This worked by another accident of how the qxl virtual gpu is designed,
> it does lazy operation evaluation on the host, so this operation
> wouldn't generally trash the memory, and result in correct display.
> 
> This tries to work out when the commit is likely just a pageflip
> and avoid touching the primary surface, this might go wrong at
> some point but I believe it's the same level as wrong as the old code
> base.
> 
> Signed-off-by: Dave Airlie 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index afbf50d..3b702d1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   struct qxl_framebuffer *qfb_old;
>   struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
>   struct qxl_bo *bo_old;
> + bool update_primary = true;
>   struct drm_clip_rect norect = {
>   .x1 = 0,
>   .y1 = 0,
> @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   if (bo == bo_old)
>   return;
>  
> + if (bo && bo_old &&
> + plane->state->crtc == old_state->crtc &&
> + plane->state->crtc_w == old_state->crtc_w &&
> + plane->state->crtc_h == old_state->crtc_h &&
> + plane->state->src_x == old_state->src_x &&
> + plane->state->src_y == old_state->src_y &&
> + plane->state->src_w == old_state->src_w &&
> + plane->state->src_h == old_state->src_h &&
> + plane->state->rotation == old_state->rotation &&
> + plane->state->zpos == old_state->zpos)
> + /* this is likely a pageflip */
> + update_primary = false;
> +
>   if (bo_old && bo_old->is_primary) {
> - qxl_io_destroy_primary(qdev);
> + if (update_primary)
> + qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
>   }
>  
>   if (!bo->is_primary) {
> - qxl_io_create_primary(qdev, 0, bo);
> + if (update_primary)
> + qxl_io_create_primary(qdev, 0, bo);
>   bo->is_primary = true;

Here the primary is still the old object but you are setting the
new.
Looking around the old is unpinned and the new one pinned which
is now wrong as QXL device suppose the memory pointer by the
primary surface (after your patch bo_old object) remains
available.

>   }
> +
>   qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, &norect, 1, 1);

I had a look at this function. It send a DRAW_COPY operation
passing the bo object as a Drawable. However does nothing to
keep the object allocated. The Drawable and its buffer (so
bo object) should be available to QXL and not touched (changed)
till a release is received.

>  }
>  

If we are not able to avoid the copy and we need to keep the old
surface in place maybe instead of creating the new object as SURFACE
we could just use for source for the Drawable for the DRAW_COPY operation.
In this way when release is received the object can be unpinned being
free to be moved.

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

[RFC PATCH 0/2] Try to smartly allocate memory on the two BARs

[Frediano Ziglio]

Currently a single type of surface is allocated in a specific BAR.
This also changes from userspace driver to the kernel one.
This way it could happen that allocation are failing even if there are
plenty of space in the other BAR.
For instance this can happen trying to change resolution as the old and
the new virtual screen is supposed to be contained in a single BAR.
The change allows allocation to occur in the BAR not being the default
for a surface type.

The patches prove to be really stable. I tested setting quite small BARs
(one or the oher) or changing default allocation BAR and continued working.
Setting large resolution is working fine while without these patches fails
for not so big BAR sizes.

Frediano Ziglio (2):
  Change the way slot is detected
  Allocate objects in both video rams

 qxl/qxl_cmd.c|  2 +-
 qxl/qxl_drv.h|  9 -
 qxl/qxl_object.c | 11 +++
 3 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.4.3

[PATCH 1/2] Change the way slot is detected

[Frediano Ziglio]

Instead of relaying on surface type use the actual placement.
This allow to have different placement for a single type of
surface.
---
 qxl/qxl_cmd.c | 2 +-
 qxl/qxl_drv.h | 9 -
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index fdc1833..3a1b055 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -511,7 +511,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
cmd->u.surface_create.height = surf->surf.height;
cmd->u.surface_create.stride = surf->surf.stride;
if (new_mem) {
-   int slot_id = surf->type == QXL_GEM_DOMAIN_VRAM ? 
qdev->main_mem_slot : qdev->surfaces_mem_slot;
+   int slot_id = qxl_bo_get_slot_id(qdev, surf);
struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);

/* TODO - need to hold one of the locks to read tbo.offset */
diff --git a/qxl/qxl_drv.h b/qxl/qxl_drv.h
index 01a8694..60f0062 100644
--- a/qxl/qxl_drv.h
+++ b/qxl/qxl_drv.h
@@ -365,11 +365,18 @@ qxl_fb_virtual_address(struct qxl_device *qdev, unsigned 
long physical)
return 0;
 }

+static inline int
+qxl_bo_get_slot_id(struct qxl_device *qdev, struct qxl_bo *bo)
+{
+   return ((bo->tbo.cur_placement & TTM_PL_MASK_MEM) == TTM_PL_FLAG_VRAM) ?
+   qdev->main_mem_slot : qdev->surfaces_mem_slot;
+}
+
 static inline uint64_t
 qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
unsigned long offset)
 {
-   int slot_id = bo->type == QXL_GEM_DOMAIN_VRAM ? qdev->main_mem_slot : 
qdev->surfaces_mem_slot;
+   int slot_id = qxl_bo_get_slot_id(qdev, bo);
struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);

/* TODO - need to hold one of the locks to read tbo.offset */
-- 
2.4.3

[PATCH 2/2] Allocate objects in both video rams

[Frediano Ziglio]

If memory is not enough in the default BAR for a type try other BAR
this allow better memory usage and avoid memory allocation failure
if a BAR is quite small and other is quite unused.
---
 qxl/qxl_object.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_object.c b/qxl/qxl_object.c
index 5e1d789..220de9a 100644
--- a/qxl/qxl_object.c
+++ b/qxl/qxl_object.c
@@ -58,14 +58,17 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 
domain, bool pinned)

qbo->placement.placement = qbo->placements;
qbo->placement.busy_placement = qbo->placements;
-   if (domain == QXL_GEM_DOMAIN_VRAM)
+   if (domain == QXL_GEM_DOMAIN_VRAM) {
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
-   if (domain == QXL_GEM_DOMAIN_SURFACE)
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_PRIV0 | pflag;
-   if (domain == QXL_GEM_DOMAIN_CPU)
+   } else if (domain == QXL_GEM_DOMAIN_SURFACE) {
+   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_PRIV0 | pflag;
+   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
+   } else if (domain == QXL_GEM_DOMAIN_CPU) {
qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
TTM_PL_FLAG_SYSTEM | pflag;
-   if (!c)
+   } else {
qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
TTM_PL_FLAG_SYSTEM;
+   }
qbo->placement.num_placement = c;
qbo->placement.num_busy_placement = c;
for (i = 0; i < c; ++i) {
-- 
2.4.3

[PATCH 04/11] drm/qxl: Avoid double free on error

[Frediano Ziglio]

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxlhw_handle_to_bo(qdev, file_priv,
   reloc.src_handle, release);
if (!reloc_info[i].src_bo) {
-   if (reloc_info[i].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base);
ret = -EINVAL;
goto out_free_bos;
}
-- 
2.1.0

[PATCH 10/11] drm/qxl: Simplify cleaning qxl processing command

[Frediano Ziglio]

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
-   if (ret) {
-   drm_gem_object_unreference_unlocked(gobj);
+   drm_gem_object_unreference_unlocked(gobj);
+   if (ret)
return NULL;
-   }

return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
struct qxl_release *release;
struct qxl_bo *cmd_bo;
void *fb_cmd;
-   int i, j, ret, num_relocs;
+   int i, ret, num_relocs;
int unwritten;

switch (cmd->type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxl_release_fence_buffer_objects(release);

 out_free_bos:
-   for (j = 0; j < num_relocs; j++) {
-   if (reloc_info[j].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
-   if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
-   }
 out_free_release:
if (ret)
qxl_release_free(qdev, release);
-- 
2.1.0

[PATCH 01/11] drm/qxl: Do not cause spice-server to clean our objects

[Frediano Ziglio]

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,

cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
cmd->type = QXL_SURFACE_CMD_CREATE;
+   cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
cmd->u.surface_create.format = surf->surf.format;
cmd->u.surface_create.width = surf->surf.width;
cmd->u.surface_create.height = surf->surf.height;
-- 
2.1.0

[PATCH 02/11] drm/qxl: Do not leak memory if qxl_release_list_add fails

[Frediano Ziglio]

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
-   if (ret)
+   if (ret) {
+   drm_gem_object_unreference_unlocked(gobj);
return NULL;
+   }

return qobj;
 }
-- 
2.1.0

[PATCH 07/11] drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved

[Frediano Ziglio]

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_release.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c 
b/drivers/gpu/drm/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,
ret = qxl_release_bo_alloc(qdev, 
&qdev->current_release_bo[cur_idx]);
if (ret) {
mutex_unlock(&qdev->release_mutex);
+   qxl_release_free(qdev, *release);
return ret;
}
}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,

mutex_unlock(&qdev->release_mutex);

-   qxl_release_list_add(*release, bo);
+   ret = qxl_release_list_add(*release, bo);
+   qxl_bo_unref(&bo);
+   if (ret) {
+   qxl_release_free(qdev, *release);
+   return ret;
+   }

info = qxl_release_map(qdev, *release);
info->id = idr_ret;
qxl_release_unmap(qdev, *release, info);

-   qxl_bo_unref(&bo);
return ret;
 }

-- 
2.1.0

[PATCH 11/11] drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

[Frediano Ziglio]

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct 
qxl_reloc_info *info)
 }

 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-struct drm_file *file_priv, uint64_t 
handle,
-struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+ struct drm_file *file_priv, uint64_t handle,
+ struct qxl_release *release, struct qxl_bo 
**qbo_p)
 {
struct drm_gem_object *gobj;
struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct 
qxl_device *qdev,

gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
if (!gobj)
-   return NULL;
+   return -EINVAL;

qobj = gem_to_qxl_bo(gobj);

ret = qxl_release_list_add(release, qobj);
drm_gem_object_unreference_unlocked(gobj);
if (ret)
-   return NULL;
+   return ret;

-   return qobj;
+   *qbo_p = qobj;
+   return 0;
 }

 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
reloc_info[i].type = reloc.reloc_type;

if (reloc.dst_handle) {
-   reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, 
file_priv,
- 
reloc.dst_handle, release);
-   if (!reloc_info[i].dst_bo) {
-   ret = -EINVAL;
-   reloc_info[i].src_bo = NULL;
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.dst_handle, release,
+&reloc_info[i].dst_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].dst_offset = reloc.dst_offset;
} else {
reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
num_relocs++;

/* reserve and validate the reloc dst bo */
-   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 
0) {
-   reloc_info[i].src_bo =
-   qxlhw_handle_to_bo(qdev, file_priv,
-  reloc.src_handle, release);
-   if (!reloc_info[i].src_bo) {
-   ret = -EINVAL;
+   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.src_handle, release,
+&reloc_info[i].src_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].src_offset = reloc.src_offset;
} else {
reloc_info[i].src_bo = NULL;
-- 
2.1.0

[PATCH 00/11 v2] Miscellaneous stability patches

[Frediano Ziglio]

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

There are no code change from v1, changes:
- fix path prefix
- rebased on drm-next branch;
- add "drm/qxl" prefix on subject;
- add reviewed by.

About which patches should be applied surely (attempting to put a priority)
- "Move main reference counter to GEM object instead of TTM ones" this can
  causes memory corruption even not wanting to;
- "Avoid double free on error" this can be cause leaks in kernel if user space
  wants, mitigated by the fact that usually DRM inodes are owned by root;
- "Handle all errors in qxl_surface_evict" could cause corruption too, not
  really probable but taking into account that Xorg implementation use a lot
  signals is not so impossible;
- "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory
  if qxl_release_list_add fails" just cause leaks on situation where memory is
  already REALLY low, can be omitted;
- "Fix print statement not using uninitialized variable", "Remove format string
  errors" should just print garbage and debugging is disabled by default, not
  necessary.

Frediano Ziglio (11):
  drm/qxl: Do not cause spice-server to clean our objects
  drm/qxl: Do not leak memory if qxl_release_list_add fails
  drm/qxl: Fix print statement not using uninitialized variable
  drm/qxl: Avoid double free on error
  drm/qxl: Handle all errors in qxl_surface_evict
  drm/qxl: Fix return for qxl_release_alloc
  drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved
  drm/qxl: Remove format string errors
  drm/qxl: Move main reference counter to GEM object instead of TTM ones
  drm/qxl: Simplify cleaning qxl processing command
  drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

 drivers/gpu/drm/qxl/qxl_cmd.c | 11 +-
 drivers/gpu/drm/qxl/qxl_display.c |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.h |  2 +-
 drivers/gpu/drm/qxl/qxl_gem.c | 10 +++--
 drivers/gpu/drm/qxl/qxl_ioctl.c   | 46 +++
 drivers/gpu/drm/qxl/qxl_object.c  | 11 --
 drivers/gpu/drm/qxl/qxl_release.c | 13 +++
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH 03/11] drm/qxl: Fix print statement not using uninitialized variable

[Frediano Ziglio]

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
/* add the bos to the list of bos to validate -
   need to validate first then process relocs? */
if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type 
!= QXL_RELOC_TYPE_SURF) {
-   DRM_DEBUG("unknown reloc type %d\n", 
reloc_info[i].type);
+   DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);

ret = -EINVAL;
goto out_free_bos;
-- 
2.1.0

[PATCH 05/11] drm/qxl: Handle all errors in qxl_surface_evict

[Frediano Ziglio]

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal
int ret;

ret = qxl_bo_reserve(surf, false);
-   if (ret == -EBUSY)
-   return -EBUSY;
+   if (ret)
+   return ret;

if (stall)
mutex_unlock(&qdev->surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal

if (stall)
mutex_lock(&qdev->surf_evict_mutex);
-   if (ret == -EBUSY) {
+   if (ret) {
qxl_bo_unreserve(surf);
-   return -EBUSY;
+   return ret;
}

qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

[PATCH 06/11] drm/qxl: Fix return for qxl_release_alloc

[Frediano Ziglio]

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c 
b/drivers/gpu/drm/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
.wait = qxl_fence_wait,
 };

-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
  struct qxl_release **ret)
 {
-- 
2.1.0

[PATCH 08/11] drm/qxl: Remove format string errors

[Frediano Ziglio]

Enable format string checks for qxl_io_log and remove resulting warnings
which could lead to memory errors on different platform or just printing
wrong information.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index b18f84c..edc1eec 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
}
}

-   QXL_INFO(qdev, "%s: %lld\n", __func__, i);
+   QXL_INFO(qdev, "%s: %d\n", __func__, i);

return i;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 4a0a8b2..a8dbb3e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
  sizeof(qdev->rom->client_monitors_config));
if (crc != qdev->rom->client_monitors_config_crc) {
-   qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
+   qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
   sizeof(qdev->rom->client_monitors_config),
   qdev->rom->client_monitors_config_crc);
return 1;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 6745c44..62ef8be 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -328,7 +328,7 @@ struct qxl_device {
 };

 /* forward declaration for QXL_INFO_IO */
-void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
+__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);

 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
diff --git a/drivers/gpu/drm/qxl/qxl_release.c 
b/drivers/gpu/drm/qxl/qxl_release.c
index 00604ed..b66ec33 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
return handle;
}
*ret = release;
-   QXL_INFO(qdev, "allocated release %lld\n", handle);
+   QXL_INFO(qdev, "allocated release %d\n", handle);
release->id = handle;
return handle;
 }
-- 
2.1.0

[PATCH 09/11] drm/qxl: Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Dave Airlie 
---
 drivers/gpu/drm/qxl/qxl_gem.c| 10 --
 drivers/gpu/drm/qxl/qxl_object.c | 11 ---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+   struct qxl_device *qdev;
+   struct ttm_buffer_object *tbo;

-   if (qobj)
-   qxl_bo_unref(&qobj);
+   qdev = (struct qxl_device *)gobj->dev->dev_private;
+
+   qxl_surface_evict(qdev, qobj, false);
+
+   tbo = &qobj->tbo;
+   ttm_bo_unref(&tbo);
 }

 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,

 void qxl_bo_unref(struct qxl_bo **bo)
 {
-   struct ttm_buffer_object *tbo;
-
if ((*bo) == NULL)
return;
-   tbo = &((*bo)->tbo);
-   ttm_bo_unref(&tbo);
-   if (tbo == NULL)
-   *bo = NULL;
+
+   drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+   *bo = NULL;
 }

 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-   ttm_bo_reference(&bo->tbo);
+   drm_gem_object_reference(&bo->gem_base);
return bo;
 }

-- 
2.1.0

[PATCH 0/2] Smartly allocate memory on the two BARs

[Frediano Ziglio]

Currently a single type of surface is allocated in a specific BAR.
This also changes from userspace driver to the kernel one.
This way it could happen that allocation are failing even if there are
plenty of space in the other BAR.
For instance this can happen trying to change resolution as the old and
the new virtual screen is supposed to be contained in a single BAR.
The change allows allocation to occur in the BAR not being the default
for a surface type.

The patches prove to be really stable. I tested setting quite small BARs
(one or the oher) or changing default allocation BAR and continued working.
Setting large resolution is working fine while without these patches fails
for not so big BAR sizes.

Changes from previous version:
- remove RFC;
- added signed-off-by;
- added prefix in commit title;
- rebased.

Frediano Ziglio (2):
  drm/qxl: change the way slot is detected
  drm/qxl: allocate objects in both video rams

 drivers/gpu/drm/qxl/qxl_cmd.c|  2 +-
 drivers/gpu/drm/qxl/qxl_drv.h|  9 -
 drivers/gpu/drm/qxl/qxl_object.c | 11 +++
 3 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.4.3

[PATCH 1/2] drm/qxl: change the way slot is detected

[Frediano Ziglio]

Instead of relaying on surface type use the actual placement.
This allow to have different placement for a single type of
surface.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_drv.h | 9 -
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index fdc1833..3a1b055 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -511,7 +511,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
cmd->u.surface_create.height = surf->surf.height;
cmd->u.surface_create.stride = surf->surf.stride;
if (new_mem) {
-   int slot_id = surf->type == QXL_GEM_DOMAIN_VRAM ? 
qdev->main_mem_slot : qdev->surfaces_mem_slot;
+   int slot_id = qxl_bo_get_slot_id(qdev, surf);
struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);

/* TODO - need to hold one of the locks to read tbo.offset */
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 01a8694..60f0062 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -365,11 +365,18 @@ qxl_fb_virtual_address(struct qxl_device *qdev, unsigned 
long physical)
return 0;
 }

+static inline int
+qxl_bo_get_slot_id(struct qxl_device *qdev, struct qxl_bo *bo)
+{
+   return ((bo->tbo.cur_placement & TTM_PL_MASK_MEM) == TTM_PL_FLAG_VRAM) ?
+   qdev->main_mem_slot : qdev->surfaces_mem_slot;
+}
+
 static inline uint64_t
 qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
unsigned long offset)
 {
-   int slot_id = bo->type == QXL_GEM_DOMAIN_VRAM ? qdev->main_mem_slot : 
qdev->surfaces_mem_slot;
+   int slot_id = qxl_bo_get_slot_id(qdev, bo);
struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);

/* TODO - need to hold one of the locks to read tbo.offset */
-- 
2.4.3

[PATCH 2/2] drm/qxl: allocate objects in both video rams

[Frediano Ziglio]

If memory is not enough in the default BAR for a type try other BAR
this allow better memory usage and avoid memory allocation failure
if a BAR is quite small and other is quite unused.

Signed-off-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_object.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b28370e..b99395e 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -58,14 +58,17 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 
domain, bool pinned)

qbo->placement.placement = qbo->placements;
qbo->placement.busy_placement = qbo->placements;
-   if (domain == QXL_GEM_DOMAIN_VRAM)
+   if (domain == QXL_GEM_DOMAIN_VRAM) {
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
-   if (domain == QXL_GEM_DOMAIN_SURFACE)
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_PRIV0 | pflag;
-   if (domain == QXL_GEM_DOMAIN_CPU)
+   } else if (domain == QXL_GEM_DOMAIN_SURFACE) {
+   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_PRIV0 | pflag;
+   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
+   } else if (domain == QXL_GEM_DOMAIN_CPU) {
qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
TTM_PL_FLAG_SYSTEM | pflag;
-   if (!c)
+   } else {
qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
TTM_PL_FLAG_SYSTEM;
+   }
qbo->placement.num_placement = c;
qbo->placement.num_busy_placement = c;
for (i = 0; i < c; ++i) {
-- 
2.4.3

[patch 3/4] drm/qxl: array underflow in qxl_clientcap_ioctl()

[Frediano Ziglio]

> 
> We check that "byte" isn't writing beyond the end of the array but we
> also need to prevent array underflow.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index b2db482..552dc06 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -372,7 +372,7 @@ static int qxl_clientcap_ioctl(struct drm_device *dev,
> void *data,
>  {
>   struct qxl_device *qdev = dev->dev_private;
>   struct drm_qxl_clientcap *param = data;
> - int byte, idx;
> + unsigned int byte, idx;
>  
>   byte = param->index / 8;
>   idx = param->index % 8;
> 

Actually there is no underflow. param->index is unsigned so either byte and
idx are at the end positive as long as int is not less then 4 bytes (which
I don't think is supported under Linux).

However I agree with the patch.

Acked!

Frediano Ziglio

[patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()

[Frediano Ziglio]

> The size calculation can overflow.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 552dc06..5da9a60 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -401,6 +401,8 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev,
> void *data,
>  
>   /* work out size allocate bo with handle */
>   actual_stride = param->stride < 0 ? -param->stride : param->stride;
> + if (actual_stride > (INT_MAX - actual_stride) / param->height)
> + return -EINVAL;
>   size = actual_stride * param->height + actual_stride;
>  
>   surf.format = param->format;
> 

There are some issues here. param->stride can be -2^31 so actual_stride will
be probably -2^31. You introduced a possible division by zero (param->height == 
0).
Do you know who validate the structure size? I don't see any check on the code.
Looks like the data pointer is coming from userspace untested.
An easiest way to do these check is use 64 bit arithmetic so something like

   u64 actual_stride, size;

   actual_stride = param->stride < 0 ? -param->stride : param->stride;
   size = actual_stride * param->height + actual_stride;
   if (size > INT_MAX)
   return -EINVAL;

take into account that usually division is quite slow so on many system even if 
32 bit
the multiplication is faster. And if is not faster you are creating a surface so
is not really a fast path.

You should also check if the stride computed from width and format fit into the 
stride
provided.

Do you know why actual_stride * param->height is not enough? Why an extra line?

Regards,
  Frediano

[patch 1/4] drm/qxl: integer overflow in qxl_process_single_command()

[Frediano Ziglio]

> 
> This size calculation can overflow on 32 bit systems leading to memory
> corruption.
> 
> Reported-by: Ilja Van Sprundel 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index bda5c5f..eda6f30 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -168,7 +168,7 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
>  cmd->command_size))
>   return -EFAULT;
>  
> - reloc_info = kmalloc(sizeof(struct qxl_reloc_info) * cmd->relocs_num,
> GFP_KERNEL);
> + reloc_info = kmalloc_array(cmd->relocs_num, sizeof(struct 
> qxl_reloc_info),
> GFP_KERNEL);
>   if (!reloc_info)
>   return -ENOMEM;
>  
> 

Looks fine.

Acked.

Frediano