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@xxxxxxxxxx> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel