[Spice-commits] 4 commits - server/display-channel.c server/display-channel.h server/red_memslots.c server/red_worker.c
Frediano Ziglio
fziglio at kemper.freedesktop.org
Thu Dec 3 15:52:35 PST 2015
server/display-channel.c | 123 +++++++++++++++++++++++++++--------------------
server/display-channel.h | 7 --
server/red_memslots.c | 4 -
server/red_worker.c | 20 -------
4 files changed, 78 insertions(+), 76 deletions(-)
New commits:
commit 1ea55284d97b1c55e2b7637ef42635d162a181c5
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Dec 1 16:46:51 2015 +0000
memslot: change some spice_assert to spice_return_if_fail
Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
diff --git a/server/red_memslots.c b/server/red_memslots.c
index 2fd939e..e883c54 100644
--- a/server/red_memslots.c
+++ b/server/red_memslots.c
@@ -168,8 +168,8 @@ void memslot_info_add_slot(RedMemSlotInfo *info, uint32_t slot_group_id, uint32_
void memslot_info_del_slot(RedMemSlotInfo *info, uint32_t slot_group_id, uint32_t slot_id)
{
- spice_assert(info->num_memslots_groups > slot_group_id);
- spice_assert(info->num_memslots > slot_id);
+ spice_return_if_fail(info->num_memslots_groups > slot_group_id);
+ spice_return_if_fail(info->num_memslots > slot_id);
info->mem_slots[slot_group_id][slot_id].virt_start_addr = 0;
info->mem_slots[slot_group_id][slot_id].virt_end_addr = 0;
commit 1873c7d497f6c32b76f71b36b41bac5bcc8c0b4d
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Dec 1 10:45:48 2015 +0000
display: misc style and rename changes
- remove some red_ prefix;
- move red_drawable->self_bitmap check outside handle_self_bitmap;
- move update check outside red_get_area (renamed surface_read_bits);
- rename depend_on_surface_id argument to surface_id;
- rename success variable to add_to_pipe.
Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Fabiano Fidêncio <fidencio at redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
diff --git a/server/display-channel.c b/server/display-channel.c
index fd3a592..2ef1c88 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -916,7 +916,7 @@ void display_channel_print_stats(DisplayChannel *display)
#endif
}
-static inline void red_inc_surfaces_drawable_dependencies(DisplayChannel *display, Drawable *drawable)
+static void drawable_ref_surface_deps(DisplayChannel *display, Drawable *drawable)
{
int x;
int surface_id;
@@ -932,23 +932,19 @@ static inline void red_inc_surfaces_drawable_dependencies(DisplayChannel *displa
}
}
-static void red_get_area(DisplayChannel *display, int surface_id, const SpiceRect *area,
- uint8_t *dest, int dest_stride, int update)
+static void surface_read_bits(DisplayChannel *display, int surface_id,
+ const SpiceRect *area, uint8_t *dest, int dest_stride)
{
SpiceCanvas *canvas;
- RedSurface *surface;
-
- surface = &display->surfaces[surface_id];
- if (update) {
- display_channel_draw(display, area, surface_id);
- }
+ RedSurface *surface = &display->surfaces[surface_id];
canvas = surface->context.canvas;
canvas->ops->read_bits(canvas, dest, dest_stride, area);
}
-static int display_channel_handle_self_bitmap(DisplayChannel *display, Drawable *drawable)
+static void handle_self_bitmap(DisplayChannel *display, Drawable *drawable)
{
+ RedDrawable *red_drawable = drawable->red_drawable;
SpiceImage *image;
int32_t width;
int32_t height;
@@ -957,20 +953,12 @@ static int display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
RedSurface *surface;
int bpp;
int all_set;
- RedDrawable *red_drawable = drawable->red_drawable;
-
- if (!red_drawable->self_bitmap) {
- return TRUE;
- }
surface = &display->surfaces[drawable->surface_id];
bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
-
- width = red_drawable->self_bitmap_area.right
- - red_drawable->self_bitmap_area.left;
- height = red_drawable->self_bitmap_area.bottom
- - red_drawable->self_bitmap_area.top;
+ width = red_drawable->self_bitmap_area.right - red_drawable->self_bitmap_area.left;
+ height = red_drawable->self_bitmap_area.bottom - red_drawable->self_bitmap_area.top;
dest_stride = SPICE_ALIGN(width * bpp, 4);
image = spice_new0(SpiceImage, 1);
@@ -989,8 +977,9 @@ static int display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
image->u.bitmap.data = spice_chunks_new_linear(dest, height * dest_stride);
image->u.bitmap.data->flags |= SPICE_CHUNKS_FLAGS_FREE;
- red_get_area(display, drawable->surface_id,
- &red_drawable->self_bitmap_area, dest, dest_stride, TRUE);
+ display_channel_draw(display, &red_drawable->self_bitmap_area, drawable->surface_id);
+ surface_read_bits(display, drawable->surface_id,
+ &red_drawable->self_bitmap_area, dest, dest_stride);
/* For 32bit non-primary surfaces we need to keep any non-zero
high bytes as the surface may be used as source to an alpha_blend */
@@ -1005,26 +994,25 @@ static int display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
}
red_drawable->self_bitmap_image = image;
- return TRUE;
}
-static inline void add_to_surface_dependency(DisplayChannel *display, int depend_on_surface_id,
+static void surface_add_reverse_dependency(DisplayChannel *display, int surface_id,
DependItem *depend_item, Drawable *drawable)
{
RedSurface *surface;
- if (depend_on_surface_id == -1) {
+ if (surface_id == -1) {
depend_item->drawable = NULL;
return;
}
- surface = &display->surfaces[depend_on_surface_id];
+ surface = &display->surfaces[surface_id];
depend_item->drawable = drawable;
ring_add(&surface->depend_on_me, &depend_item->ring_item);
}
-static inline int red_handle_surfaces_dependencies(DisplayChannel *display, Drawable *drawable)
+static int handle_surface_deps(DisplayChannel *display, Drawable *drawable)
{
int x;
@@ -1032,7 +1020,7 @@ static inline int red_handle_surfaces_dependencies(DisplayChannel *display, Draw
// surface self dependency is handled by shadows in "current", or by
// handle_self_bitmap
if (drawable->surface_deps[x] != drawable->surface_id) {
- add_to_surface_dependency(display, drawable->surface_deps[x],
+ surface_add_reverse_dependency(display, drawable->surface_deps[x],
&drawable->depend_items[x], drawable);
if (drawable->surface_deps[x] == 0) {
@@ -1136,7 +1124,7 @@ static Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t e
However, surface->depend_on_me is affected by a drawable only
as long as it is in the current tree (hasn't been rendered yet).
*/
- red_inc_surfaces_drawable_dependencies(display, drawable);
+ drawable_ref_surface_deps(display, drawable);
return drawable;
}
@@ -1147,7 +1135,7 @@ static Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t e
*/
static void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
{
- int success = FALSE, surface_id = drawable->surface_id;
+ int surface_id = drawable->surface_id;
RedDrawable *red_drawable = drawable->red_drawable;
red_drawable->mm_time = reds_get_mm_time();
@@ -1167,26 +1155,26 @@ static void display_channel_add_drawable(DisplayChannel *display, Drawable *draw
return;
}
- if (!display_channel_handle_self_bitmap(display, drawable)) {
- return;
+ if (red_drawable->self_bitmap) {
+ handle_self_bitmap(display, drawable);
}
draw_depend_on_me(display, surface_id);
- if (!red_handle_surfaces_dependencies(display, drawable)) {
+ if (!handle_surface_deps(display, drawable)) {
return;
}
Ring *ring = &display->surfaces[surface_id].current;
-
+ int add_to_pipe;
if (has_shadow(red_drawable)) {
- success = current_add_with_shadow(display, ring, drawable);
+ add_to_pipe = current_add_with_shadow(display, ring, drawable);
} else {
drawable->streamable = drawable_can_stream(display, drawable);
- success = current_add(display, ring, drawable);
+ add_to_pipe = current_add(display, ring, drawable);
}
- if (success)
+ if (add_to_pipe)
pipes_add_drawable(display, drawable);
#ifdef RED_WORKER_STAT
@@ -1211,7 +1199,6 @@ void display_channel_process_draw(DisplayChannel *display, RedDrawable *red_draw
display_channel_drawable_unref(display, drawable);
}
-
int display_channel_wait_for_migrate_data(DisplayChannel *display)
{
uint64_t end_time = red_get_monotonic_time() + DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
commit 1940972ca9f0ab70733804506e1ae400351a4dcb
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Mon Nov 30 18:19:30 2015 +0000
worker: move red_process_draw to display-channel.c
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Fabiano Fidêncio <fidencio at redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
diff --git a/server/display-channel.c b/server/display-channel.c
index 7aa54ed..fd3a592 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1098,9 +1098,9 @@ static int validate_drawable_bbox(DisplayChannel *display, RedDrawable *drawable
*
* @return initialized Drawable or NULL on failure
*/
-Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
- RedDrawable *red_drawable, uint32_t group_id,
- uint32_t process_commands_generation)
+static Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
+ RedDrawable *red_drawable, uint32_t group_id,
+ uint32_t process_commands_generation)
{
Drawable *drawable;
int x;
@@ -1145,7 +1145,7 @@ Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
* Add a Drawable to the items to draw.
* On failure the Drawable is not added.
*/
-void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
+static void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
{
int success = FALSE, surface_id = drawable->surface_id;
RedDrawable *red_drawable = drawable->red_drawable;
@@ -1195,6 +1195,23 @@ void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
#endif
}
+void display_channel_process_draw(DisplayChannel *display, RedDrawable *red_drawable,
+ uint32_t group_id, int process_commands_generation)
+{
+ Drawable *drawable =
+ display_channel_get_drawable(display, red_drawable->effect, red_drawable, group_id,
+ process_commands_generation);
+
+ if (!drawable) {
+ return;
+ }
+
+ display_channel_add_drawable(display, drawable);
+
+ display_channel_drawable_unref(display, drawable);
+}
+
+
int display_channel_wait_for_migrate_data(DisplayChannel *display)
{
uint64_t end_time = red_get_monotonic_time() + DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
diff --git a/server/display-channel.h b/server/display-channel.h
index 83b50ca..a990e09 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -286,8 +286,6 @@ void display_channel_surface_unref (DisplayCha
uint32_t surface_id);
bool display_channel_surface_has_canvas (DisplayChannel *display,
uint32_t surface_id);
-void display_channel_add_drawable (DisplayChannel *display,
- Drawable *drawable);
void display_channel_current_flush (DisplayChannel *display,
int surface_id);
int display_channel_wait_for_migrate_data (DisplayChannel *display);
@@ -300,11 +298,10 @@ void display_channel_destroy_surfaces (DisplayCha
void display_channel_destroy_surface (DisplayChannel *display,
uint32_t surface_id);
uint32_t display_channel_generate_uid (DisplayChannel *display);
-Drawable * display_channel_get_drawable (DisplayChannel *display,
- uint8_t effect,
+void display_channel_process_draw (DisplayChannel *display,
RedDrawable *red_drawable,
uint32_t group_id,
- uint32_t process_commands_generation);
+ int process_commands_generation);
void display_channel_process_surface_cmd (DisplayChannel *display,
RedSurfaceCmd *surface,
uint32_t group_id,
diff --git a/server/red_worker.c b/server/red_worker.c
index 13a3ec2..b5bab09 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -165,23 +165,6 @@ void red_drawable_unref(RedWorker *worker, RedDrawable *red_drawable,
free(red_drawable);
}
-static inline void red_process_draw(RedWorker *worker, RedDrawable *red_drawable,
- uint32_t group_id)
-{
- DisplayChannel *display = worker->display_channel;
- Drawable *drawable =
- display_channel_get_drawable(display, red_drawable->effect, red_drawable, group_id,
- worker->process_display_generation);
-
- if (!drawable) {
- return;
- }
-
- display_channel_add_drawable(worker->display_channel, drawable);
-
- display_channel_drawable_unref(display, drawable);
-}
-
static int red_process_cursor(RedWorker *worker, uint32_t max_pipe_size, int *ring_is_empty)
{
QXLCommandExt ext_cmd;
@@ -284,7 +267,8 @@ static int red_process_display(RedWorker *worker, uint32_t max_pipe_size, int *r
if (!red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
red_drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
- red_process_draw(worker, red_drawable, ext_cmd.group_id);
+ display_channel_process_draw(worker->display_channel, red_drawable, ext_cmd.group_id,
+ worker->process_display_generation);
}
// release the red_drawable
red_drawable_unref(worker, red_drawable, ext_cmd.group_id);
commit dfaf83235f3021f8870a2bdee4820f43930b7ddd
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Mon Nov 30 18:08:59 2015 +0000
display: make get_drawable symmetric to display_channel_drawable_unref
Make possible to safely call display_channel_drawable_unref straight
after calling get_drawable.
Problem was function definitions and dependency.
display_channel_drawable_try_new is supposed to return an uninitialized
pointer (or NULL on failure) to a Drawable structure.
(display_channel_)get_drawable is supposed to return an initialized
pointer (or NULL) to a Drawable structure.
(display_channel_)add_drawable is supposed to add the Drawable to the
list/tree of drawing to draw.
Now, with these definitions after get_drawable the Drawable state (if
pointer is not NULL) should be consistent and we should be able to call
display_channel_drawable_unref.
In the current code this was not true as for instance surface_id was
copied to Drawable but the reference counter of the surface was not
incremented leading possible unref call to decrement the counter and
free the surface. This can happen if any call between get_drawable and
unref does not increment the reference in a consistent way. This
basically means that every present or future code in the path between
get_drawable and unref have to know this unconsistency and handle it.
This is a maintaining problem as people need to know these details when
editing existing code (actually this is display_channel_add_drawable
code).
This basically create a dependency between get_drawable and
add_drawable.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
diff --git a/server/display-channel.c b/server/display-channel.c
index 7017b2a..7aa54ed 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1091,6 +1091,13 @@ static int validate_drawable_bbox(DisplayChannel *display, RedDrawable *drawable
return TRUE;
}
+/**
+ * @brief Get a new Drawable
+ *
+ * The Drawable returned is fully initialized.
+ *
+ * @return initialized Drawable or NULL on failure
+ */
Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
RedDrawable *red_drawable, uint32_t group_id,
uint32_t process_commands_generation)
@@ -1098,6 +1105,9 @@ Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
Drawable *drawable;
int x;
+ /* Validate all surface ids before updating counters
+ * to avoid invalid updates if we find an invalid id.
+ */
if (!validate_drawable_bbox(display, red_drawable)) {
return NULL;
}
@@ -1117,20 +1127,30 @@ Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
drawable->red_drawable = red_drawable_ref(red_drawable);
drawable->surface_id = red_drawable->surface_id;
+ display->surfaces[drawable->surface_id].refs++;
+
memcpy(drawable->surface_deps, red_drawable->surface_deps, sizeof(drawable->surface_deps));
+ /*
+ surface->refs is affected by a drawable (that is
+ dependent on the surface) as long as the drawable is alive.
+ However, surface->depend_on_me is affected by a drawable only
+ as long as it is in the current tree (hasn't been rendered yet).
+ */
+ red_inc_surfaces_drawable_dependencies(display, drawable);
return drawable;
}
+/**
+ * Add a Drawable to the items to draw.
+ * On failure the Drawable is not added.
+ */
void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
{
int success = FALSE, surface_id = drawable->surface_id;
RedDrawable *red_drawable = drawable->red_drawable;
red_drawable->mm_time = reds_get_mm_time();
- surface_id = drawable->surface_id;
-
- display->surfaces[surface_id].refs++;
region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
@@ -1143,14 +1163,6 @@ void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
region_destroy(&rgn);
}
- /*
- surface->refs is affected by a drawable (that is
- dependent on the surface) as long as the drawable is alive.
- However, surface->depend_on_me is affected by a drawable only
- as long as it is in the current tree (hasn't been rendered yet).
- */
- red_inc_surfaces_drawable_dependencies(display, drawable);
-
if (region_is_empty(&drawable->tree_item.base.rgn)) {
return;
}
@@ -1345,6 +1357,11 @@ static void drawables_init(DisplayChannel *display)
}
}
+/**
+ * Allocate a Drawable
+ *
+ * @return pointer to uninitialized Drawable or NULL on failure
+ */
Drawable *display_channel_drawable_try_new(DisplayChannel *display,
int group_id, int process_commands_generation)
{
More information about the Spice-commits
mailing list