[Spice-devel,0/2] stability for dual head

Submitted by David Mansfield on June 16, 2014, 1:16 p.m.

Details

Message ID 539EEE3D.9070602@dm.cobite.com
State New
Headers show

Not browsing as part of any series.

Commit Message

David Mansfield June 16, 2014, 1:16 p.m.
On 06/09/2014 09:29 AM, Alon Levy wrote:
> On 06/09/2014 04:18 PM, David Mansfield wrote:
>> On 06/09/2014 07:18 AM, Alon Levy wrote:
>>> On 06/03/2014 04:14 PM, David Mansfield wrote:
>>>> Bump. I'll make it easy.  This is a multiple choice response form.
>>>> Anyone reading this can respond with one letter so save time and effort.
>>>>
>>>> a) "We're too busy with RHEL 7/paying clients, come back in a month/some
>>>> timeframe"
>>>> b) "There's an SEP field on these problems, everyone who understands
>>>> that code has moved on"
>>>> c) "Go away"
>>>> d) "Oops, I've been meaning to get back to you but I keep forgetting and
>>>> life is hectic..."
>>>> e) "Didn't you hear? SPICE is dead."
>>>> f) "Other." Please elaborate using the space provided below:
>>> The first patch looks good (just adjusting the #if to disable the
>>> print). I'll pick it up, thanks, you deserved a faster response.
>>>
>>> No idea what SEP is.
>> Hi Alon,
>>
>> I followed Marc-André's advice and sent these out to DRI ond xorg
>> mailing lists, respectively.  The qxl.ko patch was picked up by Dave
>> Airlie and committed to drm-next branch.
>>
>> The second is still without a home.
>>
>> (BTW: An SEP is a "somebody else's problem" effect, see
>> http://en.wikipedia.org/wiki/Somebody_Else%27s_Problem, popularized in
>> Douglas Adams' Hitchhiker's Guide novel.  Very funny concept.)
> Missed that.
>
>> Any possibility of help with issue #2, the xorg-devel list is silent on
>> this  one and I don't know who the maintainer is specifically. Without
>> this patch xorg-qxl is trivially crashable when using dual head at
>> 1920x1200 resolution (or potentially lower resolution).
>>
> 96 relocs with 512x512 chunks - what do you do to crash it?
>
> Soren Sandmann is the maintainer, but I did a release once, I can commit
> it once I'm done testing (need to allow large resolutions which by
> default are limited to surface0_area_size).

Good morning (evening?) Alon,

Anything else you need from me on this? I've attached the patch from 
git-format-patch that should have all of the correct signed-off etc. and 
a proper commit description.

Just a friendly ping.

Patch hide | download patch | download mbox

From c68706dcd3c868d9baff2461413cadf6201eee8a Mon Sep 17 00:00:00 2001
From: David Mansfield <spice@dm.cobite.com>
Date: Tue, 3 Jun 2014 10:05:42 -0400
Subject: [PATCH qxl] Dynamically adjust chunk size to avoid command buffer
 overflow.

The maximum number of "commands" that can be queued at once is
fixed at compile time at MAX_RELOCS. However, during the creation
of an image object in qxl_image_create(), the image is split into
commands of maximum size 512*512. For a large dual-head system,
it is easy to create an image for which the number of chunks will
result in an overflow of MAX_RELOCS number of "commands".

Identify this scenario and dynamically increase the chunk size to
avoid the overflow, and the resulting assert() which crashes Xorg.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=79317
Signed-off-by: David Mansfield <spice@dm.cobite.com>
---
 src/qxl_image.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/qxl_image.c b/src/qxl_image.c
index 0a0ca30..396aa0f 100644
--- a/src/qxl_image.c
+++ b/src/qxl_image.c
@@ -140,6 +140,7 @@  qxl_image_create (qxl_screen_t *qxl, const uint8_t *data,
 	struct qxl_bo *image_bo;
 	int dest_stride = (width * Bpp + 3) & (~3);
 	int h;
+	int chunk_size;
 
 	data += y * stride + x * Bpp;
 
@@ -155,9 +156,23 @@  qxl_image_create (qxl_screen_t *qxl, const uint8_t *data,
 
 	hash = 0;
 	h = height;
+
+	chunk_size = MAX (512 * 512, dest_stride);
+
+	/* ensure we will not create too many pieces and overflow
+	 * the command buffer (MAX_RELOCS).  if so, increase the chunk_size.
+	 * each loop creates at least 2 cmd buffer entries, and 
+	 * we have to leave room when we're done.
+	 */
+	if (height / (chunk_size / dest_stride) > (MAX_RELOCS / 4)) {
+		chunk_size = height / (MAX_RELOCS/4) * dest_stride;
+#if 0
+		ErrorF ("adjusted chunk_size to %d\n", chunk_size);
+#endif
+	}
+
 	while (h)
 	{
-	    int chunk_size = MAX (512 * 512, dest_stride);
 	    int n_lines = MIN ((chunk_size / dest_stride), h);
 	    struct qxl_bo *bo = qxl->bo_funcs->bo_alloc (qxl, sizeof (QXLDataChunk) + n_lines * dest_stride, "image data");
 
-- 
1.9.0



Comments

On 06/16/2014 04:16 PM, David Mansfield wrote:
> 
> On 06/09/2014 09:29 AM, Alon Levy wrote:
>> On 06/09/2014 04:18 PM, David Mansfield wrote:
>>> On 06/09/2014 07:18 AM, Alon Levy wrote:
>>>> On 06/03/2014 04:14 PM, David Mansfield wrote:
>>>>> Bump. I'll make it easy.  This is a multiple choice response form.
>>>>> Anyone reading this can respond with one letter so save time and
>>>>> effort.
>>>>>
>>>>> a) "We're too busy with RHEL 7/paying clients, come back in a
>>>>> month/some
>>>>> timeframe"
>>>>> b) "There's an SEP field on these problems, everyone who understands
>>>>> that code has moved on"
>>>>> c) "Go away"
>>>>> d) "Oops, I've been meaning to get back to you but I keep
>>>>> forgetting and
>>>>> life is hectic..."
>>>>> e) "Didn't you hear? SPICE is dead."
>>>>> f) "Other." Please elaborate using the space provided below:
>>>> The first patch looks good (just adjusting the #if to disable the
>>>> print). I'll pick it up, thanks, you deserved a faster response.
>>>>
>>>> No idea what SEP is.
>>> Hi Alon,
>>>
>>> I followed Marc-André's advice and sent these out to DRI ond xorg
>>> mailing lists, respectively.  The qxl.ko patch was picked up by Dave
>>> Airlie and committed to drm-next branch.
>>>
>>> The second is still without a home.
>>>
>>> (BTW: An SEP is a "somebody else's problem" effect, see
>>> http://en.wikipedia.org/wiki/Somebody_Else%27s_Problem, popularized in
>>> Douglas Adams' Hitchhiker's Guide novel.  Very funny concept.)
>> Missed that.
>>
>>> Any possibility of help with issue #2, the xorg-devel list is silent on
>>> this  one and I don't know who the maintainer is specifically. Without
>>> this patch xorg-qxl is trivially crashable when using dual head at
>>> 1920x1200 resolution (or potentially lower resolution).
>>>
>> 96 relocs with 512x512 chunks - what do you do to crash it?
>>
>> Soren Sandmann is the maintainer, but I did a release once, I can commit
>> it once I'm done testing (need to allow large resolutions which by
>> default are limited to surface0_area_size).
> 
> Good morning (evening?) Alon,
> 
> Anything else you need from me on this? I've attached the patch from
> git-format-patch that should have all of the correct signed-off etc. and
> a proper commit description.
> 
> Just a friendly ping.
> 

Thanks. I pushed your patch (plus just a white space fix).

It is still not testable without changing the surface0 allocation
manually or enabling surface resizing. Have you taken a look perhaps at
the surface resize stuff? how did you test it so far?
On 06/17/2014 03:24 AM, Alon Levy wrote:
> On 06/16/2014 04:16 PM, David Mansfield wrote:
>> On 06/09/2014 09:29 AM, Alon Levy wrote:
>>> On 06/09/2014 04:18 PM, David Mansfield wrote:
>>>> On 06/09/2014 07:18 AM, Alon Levy wrote:
>>>>> On 06/03/2014 04:14 PM, David Mansfield wrote:
>>>>>> Bump. I'll make it easy.  This is a multiple choice response form.
>>>>>> Anyone reading this can respond with one letter so save time and
>>>>>> effort.
>>>>>>
>>>>>> a) "We're too busy with RHEL 7/paying clients, come back in a
>>>>>> month/some
>>>>>> timeframe"
>>>>>> b) "There's an SEP field on these problems, everyone who understands
>>>>>> that code has moved on"
>>>>>> c) "Go away"
>>>>>> d) "Oops, I've been meaning to get back to you but I keep
>>>>>> forgetting and
>>>>>> life is hectic..."
>>>>>> e) "Didn't you hear? SPICE is dead."
>>>>>> f) "Other." Please elaborate using the space provided below:
>>>>> The first patch looks good (just adjusting the #if to disable the
>>>>> print). I'll pick it up, thanks, you deserved a faster response.
>>>>>
>>>>> No idea what SEP is.
>>>> Hi Alon,
>>>>
>>>> I followed Marc-André's advice and sent these out to DRI ond xorg
>>>> mailing lists, respectively.  The qxl.ko patch was picked up by Dave
>>>> Airlie and committed to drm-next branch.
>>>>
>>>> The second is still without a home.
>>>>
>>>> (BTW: An SEP is a "somebody else's problem" effect, see
>>>> http://en.wikipedia.org/wiki/Somebody_Else%27s_Problem, popularized in
>>>> Douglas Adams' Hitchhiker's Guide novel.  Very funny concept.)
>>> Missed that.
>>>
>>>> Any possibility of help with issue #2, the xorg-devel list is silent on
>>>> this  one and I don't know who the maintainer is specifically. Without
>>>> this patch xorg-qxl is trivially crashable when using dual head at
>>>> 1920x1200 resolution (or potentially lower resolution).
>>>>
>>> 96 relocs with 512x512 chunks - what do you do to crash it?
>>>
>>> Soren Sandmann is the maintainer, but I did a release once, I can commit
>>> it once I'm done testing (need to allow large resolutions which by
>>> default are limited to surface0_area_size).
>> Good morning (evening?) Alon,
>>
>> Anything else you need from me on this? I've attached the patch from
>> git-format-patch that should have all of the correct signed-off etc. and
>> a proper commit description.
>>
>> Just a friendly ping.
>>
> Thanks. I pushed your patch (plus just a white space fix).
>
> It is still not testable without changing the surface0 allocation
> manually or enabling surface resizing. Have you taken a look perhaps at
> the surface resize stuff? how did you test it so far?
Not sure what you mean by surface0 allocation, but here's my environment 
where I can 100% reproduce this issue:

- F20 host ,F20 guest, F20 client (all same box)
- In the libvirt domain xml, a few tweks to increase memory available to 
qxl video framebuffer from 16mb to 32mb in domain xml:

     <video>
       <model type='qxl' ram='131072' vram='131072' heads='1'/>
     </video>

   <qemu:commandline>
     <qemu:arg value='-global'/>
     <qemu:arg value='qxl-vga.vgamem_mb=32'/>
   </qemu:commandline>

- run MATE desktop in guest, not sure if this is necessary or not, but 
it's what I use
- use two (virtual and physical) monitors with 1920x1200 resolution (24 
bit color depth). You will need either a patched kernel (or a 3.16 
kernel), or a patched remote-viewer because of a different bug which is 
fixed in qxl.ko in latest kernel or else the second monitor may show 
"entire" framebuffer instead of monitor 2 area.

At this point "shutter" will crash the system when doing a screenshot 
using the "selection" feature, as will opening the second virtual 
monitor with the saved monitor configuration in .config/monitors.xml.

I appreciate your help getting this in!

Thanks,
David
On 06/17/2014 04:08 PM, David Mansfield wrote:
> 
> On 06/17/2014 03:24 AM, Alon Levy wrote:
>> On 06/16/2014 04:16 PM, David Mansfield wrote:
>>> On 06/09/2014 09:29 AM, Alon Levy wrote:
>>>> On 06/09/2014 04:18 PM, David Mansfield wrote:
>>>>> On 06/09/2014 07:18 AM, Alon Levy wrote:
>>>>>> On 06/03/2014 04:14 PM, David Mansfield wrote:
>>>>>>> Bump. I'll make it easy.  This is a multiple choice response form.
>>>>>>> Anyone reading this can respond with one letter so save time and
>>>>>>> effort.
>>>>>>>
>>>>>>> a) "We're too busy with RHEL 7/paying clients, come back in a
>>>>>>> month/some
>>>>>>> timeframe"
>>>>>>> b) "There's an SEP field on these problems, everyone who understands
>>>>>>> that code has moved on"
>>>>>>> c) "Go away"
>>>>>>> d) "Oops, I've been meaning to get back to you but I keep
>>>>>>> forgetting and
>>>>>>> life is hectic..."
>>>>>>> e) "Didn't you hear? SPICE is dead."
>>>>>>> f) "Other." Please elaborate using the space provided below:
>>>>>> The first patch looks good (just adjusting the #if to disable the
>>>>>> print). I'll pick it up, thanks, you deserved a faster response.
>>>>>>
>>>>>> No idea what SEP is.
>>>>> Hi Alon,
>>>>>
>>>>> I followed Marc-André's advice and sent these out to DRI ond xorg
>>>>> mailing lists, respectively.  The qxl.ko patch was picked up by Dave
>>>>> Airlie and committed to drm-next branch.
>>>>>
>>>>> The second is still without a home.
>>>>>
>>>>> (BTW: An SEP is a "somebody else's problem" effect, see
>>>>> http://en.wikipedia.org/wiki/Somebody_Else%27s_Problem, popularized in
>>>>> Douglas Adams' Hitchhiker's Guide novel.  Very funny concept.)
>>>> Missed that.
>>>>
>>>>> Any possibility of help with issue #2, the xorg-devel list is
>>>>> silent on
>>>>> this  one and I don't know who the maintainer is specifically. Without
>>>>> this patch xorg-qxl is trivially crashable when using dual head at
>>>>> 1920x1200 resolution (or potentially lower resolution).
>>>>>
>>>> 96 relocs with 512x512 chunks - what do you do to crash it?
>>>>
>>>> Soren Sandmann is the maintainer, but I did a release once, I can
>>>> commit
>>>> it once I'm done testing (need to allow large resolutions which by
>>>> default are limited to surface0_area_size).
>>> Good morning (evening?) Alon,
>>>
>>> Anything else you need from me on this? I've attached the patch from
>>> git-format-patch that should have all of the correct signed-off etc. and
>>> a proper commit description.
>>>
>>> Just a friendly ping.
>>>
>> Thanks. I pushed your patch (plus just a white space fix).
>>
>> It is still not testable without changing the surface0 allocation
>> manually or enabling surface resizing. Have you taken a look perhaps at
>> the surface resize stuff? how did you test it so far?
> Not sure what you mean by surface0 allocation,

I'm referring to some currently disabled (preprocessor macro) code, but
on second thought it is not required.

> but here's my environment
> where I can 100% reproduce this issue:
> 
> - F20 host ,F20 guest, F20 client (all same box)
> - In the libvirt domain xml, a few tweks to increase memory available to
> qxl video framebuffer from 16mb to 32mb in domain xml:
> 
>     <video>
>       <model type='qxl' ram='131072' vram='131072' heads='1'/>
>     </video>
> 
>   <qemu:commandline>
>     <qemu:arg value='-global'/>
>     <qemu:arg value='qxl-vga.vgamem_mb=32'/>
>   </qemu:commandline>
> 
> - run MATE desktop in guest, not sure if this is necessary or not, but
> it's what I use
> - use two (virtual and physical) monitors with 1920x1200 resolution (24
> bit color depth). You will need either a patched kernel (or a 3.16
> kernel), or a patched remote-viewer because of a different bug which is
> fixed in qxl.ko in latest kernel or else the second monitor may show
> "entire" framebuffer instead of monitor 2 area.
> 
> At this point "shutter" will crash the system when doing a screenshot
> using the "selection" feature, as will opening the second virtual
> monitor with the saved monitor configuration in .config/monitors.xml.
> 
> I appreciate your help getting this in!

Still need to do a release.

> 
> Thanks,
> David
> 
>