[Spice-devel] experimental spice-server branch

[Marc-André Lureau]

Hi

Some early comments while reading, since I haven't tested this yet
(and doesn't really know how to measure all of that either)

On Thu, Oct 24, 2013 at 5:29 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> http://cgit.freedesktop.org/~yhalperi/spice/
> branch bitmap-crop.mem-control

- "red_worker/fill_bits: separate filling & encoding bitmap", is it ok
if it doesn't return FILL_BITS_TYPE_BITMAP anymore?

- "separate filling quic SpiceImage" looks suspicious, because it is
doing more in spice_image_set_quic_data() than before.

- "activate bitmap cropping", why not just pass a "&crop_src_bitmap"
to fill_bits? Or have an alternative fill_bits_cropped?

- "red_parse_qxl: track qxl resources size", I guess it wouldn't need
to pass qxl_res everywhere if simply using container_of macro. I also
worry about keeping pointers to SpiceImages when all we want is to
track the size? but the following patch explains why.

- "tracking the size of the occupied dev ram space", a bit of
background could help. I wonder why it's not possible to add an
interface for driver to give an estimation of memory usage. It looks
like this could potentially be expensive and approximative in server.
btw, there are tricks to use ghashtable as a set (see ghashtable doc),
the hit_count could be added to RedQXLResources or RedDrawable.

- "control calls to QXLInterface->flush_resources" Isn't the result of
calling flush_resources depending on what is in the release ring? It
will just try to release a bit more often, but it will not release
more than what OOM would do, right? If it's the case, I don't see how
it can help.

- "improve OOM handling" brrr.. :) quite complicated sutff... "makes
handle_dev_oom take into consideration our current estimation..." I
don't get what is really improved or changed though. It tries to
release at least 10% of held memory with this patch? Wouldn't it make
sense to avoid the estimation and just try to release 10% of ram_size?

- "trigger release of drawables when the estimated memory", same
doubts as before about usefulness of this. I would wait for some
tests/measures.

- "support releasing drawables that are referenced" looks interesting.
I wonder if some ops couldn't be simply copied (that don't reference
other surfaces). Also why do you release pipe item in both
surface_pipe_draw_data_list_add_draw_info() and
red_display_replace_surface_draw_data_with_image(). Why not always in
surface_pipe_draw_data_list_add_draw_info()? Do you know how well the
compression performs when the drawing has non-rectangle regions? Is it
re-divided in rectangular regions?

cheers

-- 
Marc-André Lureau