[Spice-devel] [PATCH spice-server 1/3] red_worker: cleanup red_clear_surface_drawables_from_pipes
Marc-André Lureau
marcandre.lureau at gmail.com
Wed Sep 18 06:19:36 PDT 2013
Hi
On Wed, Sep 18, 2013 at 2:58 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> Hi,
>
>
> On 09/17/2013 12:11 PM, Marc-André Lureau wrote:
>>
>> On Mon, Sep 16, 2013 at 5:34 PM, Yonit Halperin <yhalperi at redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> 'force' means: wait till there is no pipe item that references the
>>> surface.
>>> If force = FALSE, try to release any such pipe item, but as long as it
>>> doesn't require blocking.
>>>
>>>
>>> On 09/16/2013 09:00 AM, Marc-André Lureau wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> Since I don't understand the "force" argument, it would be easier to
>>>> review to keep current behaviour and just call
>>>> red_wait_outgoing_item() at the end.
>>>>
>>>> To me the meaning of "force" argument is rather "do not wait if dep".
>>>> When a drawable depend on the surface:
>>>> - force == false: return immediately when found
>>>> - force == true: wait for the first found item to be sent (I guess it
>>>> is the last to be sent, but isn't it supposed to be sent before the
>>>> one that depends on it?)
>>>
>>>
>>> red_clear_surface_drawables_from_pipe(surface=S) scans the pipe from the
>>> newest item to the oldest item (the first to be sent). It removes the
>>> newer
>>> pipe items that are directly drawn over surface S, till it finds an item
>>> that depends on S (i.e., S is its src/mask/brush). Such item might
>>> require
>>> that older pipe items that are drawn over S will be rendered on the
>>> client
>>> side. Thus, if force=TRUE, we wait till the newest item that depends on S
>>> is
>>> sent to the client.
>>
>>
>> Ok, that's pretty clear in the code. However why is it called "force"?
>> It's rather "wait if used" (trying to find an appropriate name).
>
> I can rename it to block_till_done or something like that
Please keep the "wait" in the name, since it calls "wait_item"
>>
>>> red_wait_outgoing_item() is called when a dependent item is not found,
>>> for
>>> case that the last item that was dequeued from the pipe, and is currently
>>> during sending, refers to surface S (we could have had a method that
>>> returns
>>> the currently sent item, and check it, but we don't have one yet)...
>>> I can add this comments to the code if it helps.
>>
>>
>> Ok, I still think it would be simpler if it wouldn't depend on this
>> weird "if (force)" conditionnal block. Ie, do it unconditionnally,
>> like before, with a comment explaining why it is necessary. (although
>
> I wouldn't call it weird. We don't want to block if the "force" wasn't set
> to TRUE (which happens quite often when off-screen surfaces are enabled; for
> every destroy_surface command). Also, before the patch it was also called
> just in case "if (wait_for_outgoing_item)"
I think it would be easier to read written like this:
if (!wait)
return;
if (item)
wait_pipe_item()
else
red_wait_outgoing_item()
but the last call "just in case" clearly indicate it is somehow misplaced.
>> probably this wait should be done before or during
>> pipe_remove_and_release..)
>
> This call need to be done once, for the item that we are currently in the
> middle of sending. I don't think it is misplaced.
After a call to something called
red_channel_client_pipe_remove_and_release, I expect to be done with
it, not to care later about it being still around. That's why I think
it's misplaced.
I could still "be done once" when necessary, even inside
remove_and_release. I haven't looked at the details though.
>>
>>>>
>>>> If you could clarified the above, and comment it in the code that
>>>> would be helpful! :)
>>>>
>>>>
>>>> On Thu, Sep 12, 2013 at 10:04 PM, Yonit Halperin <yhalperi at redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> (1) merge 'force' and 'wait_for_outgoing_item' to one parameter.
>>>>> 'wait_for_outgoing_item' is a derivative of 'force'.
>>>>> (2) move the call to red_wait_outgoing_item to
>>>>> red_clear_surface_drawables_from_pipe
>>>>> ---
>>>>> server/red_worker.c | 20 +++++++++++---------
>>>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>>>> index 0c611d0..e9a7fa1 100644
>>>>> --- a/server/red_worker.c
>>>>> +++ b/server/red_worker.c
>>>>> @@ -2064,22 +2064,24 @@ static void
>>>>> red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
>>>>>
>>>>> if (item) {
>>>>> red_channel_client_wait_pipe_item_sent(&dcc->common.base,
>>>>> item);
>>>>> + } else if (force) {
>>>>> + /*
>>>>> + * in case that the pipe didn't contain any item that is
>>>>> dependent on the surface, but
>>>>> + * there is one during sending.
>>>>> + */
>>>>> + red_wait_outgoing_item(&dcc->common.base);
>>>>> }
>>>>> }
>>>>>
>>>>> -static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>>>> int surface_id,
>>>>> - int force, int wait_for_outgoing_item)
>>>>> +static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>>>> + int surface_id,
>>>>> + int force)
>>>>> {
>>>>> RingItem *item, *next;
>>>>> DisplayChannelClient *dcc;
>>>>>
>>>>> WORKER_FOREACH_DCC_SAFE(worker, item, next, dcc) {
>>>>> red_clear_surface_drawables_from_pipe(dcc, surface_id,
>>>>> force);
>>>>> - if (wait_for_outgoing_item) {
>>>>> - // in case that the pipe didn't contain any item that is
>>>>> dependent on the surface, but
>>>>> - // there is one during sending.
>>>>> - red_wait_outgoing_item(&dcc->common.base);
>>>>> - }
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -4248,7 +4250,7 @@ static inline void red_process_surface(RedWorker
>>>>> *worker, RedSurfaceCmd *surface
>>>>> otherwise "current" will hold items that other drawables
>>>>> may
>>>>> depend on, and then
>>>>> red_current_clear will remove them from the pipe. */
>>>>> red_current_clear(worker, surface_id);
>>>>> - red_clear_surface_drawables_from_pipes(worker, surface_id,
>>>>> FALSE, FALSE);
>>>>> + red_clear_surface_drawables_from_pipes(worker, surface_id,
>>>>> FALSE);
>>>>> red_destroy_surface(worker, surface_id);
>>>>> break;
>>>>> default:
>>>>> @@ -10921,7 +10923,7 @@ static inline void
>>>>> destroy_surface_wait(RedWorker
>>>>> *worker, int surface_id)
>>>>> otherwise "current" will hold items that other drawables may
>>>>> depend on, and then
>>>>> red_current_clear will remove them from the pipe. */
>>>>> red_current_clear(worker, surface_id);
>>>>> - red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE,
>>>>> TRUE);
>>>>> + red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE);
>>>>> }
>>>>>
>>>>> static void dev_destroy_surface_wait(RedWorker *worker, uint32_t
>>>>> surface_id)
>>>>> --
>>>>> 1.8.1.4
>>>>>
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>
--
Marc-André Lureau
More information about the Spice-devel
mailing list