[weston 1/2] linux-dmabuf: implement immediate dmabuf import

[Daniel Stone]

Hi Pekka,

On 30 January 2017 at 12:51, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Mon, 30 Jan 2017 16:00:06 +0530
> Varad Gautam <varadgautam at gmail.com> wrote:
>> Instead of always always creating a wl_resource and identifying an invalid
>> wl_buffer on commit in weston, how about treating an invalid import just as
>> any other cause for failure in linux-dmabuf, and letting the
>> compositor error-out
>> at import itself?
>>
>> So, upon failed import the implementation can choose to either:
>> 1. raise an error and never create an invalid resource; or,
>> 2. create an invalid resource and have its own checks in the commit path
>> (or elsewhere) to decide how to handle the situation.
>>
>> Option 1 allows us to have a 'semi-defined' behavior for existing compositors
>> {weston} that expect a commit guarantee on wl_buffers. The protocol v2 [1]
>> added an optional invalid_wl_buffer error, to be preventively used by
>> compositors
>> that mandate the committed wl_buffers to be valid to handle the failed import
>> case.
>
> I had a long chat with Daniel on #wayland today. There are many ways to
> design this, and none seem to have compelling reasons for or against,
> so sorry for the back and forth. Your suggestion seems good.
>
>
> Starting from the top, there are two use cases:
>
> a) The safe way, recommended: client sends the create request and needs
>    to wait for an event before attaching and committing the wl_buffer.
>
> b) The fast way, for special platforms where success is "guaranteed":
>    client sends all the create request, attach and commit in a single go
>    without waiting for anything.
>
> I can't really make my mind on how to formulate the protocol, but
> Daniel had the following very slight preference: separate create
> requests with a common error event for graceful errors.
>
> So, we keep the existing protocol, not aiming to deprecate it.
>
> Then we add 'create_immed' with the signature you defined for it. If
> 'create_immed' succeeds, there is no event sent. If the import by
> 'create_immed' fails, the compositor has two options: either it sends a
> fatal protocol error, or it sends the 'failed' event. The choice is
> implementation defined.
>
> If the compositor sends 'failed' event, it is undefined what happens if
> the client still tries to use the wl_buffer.
>
> Weston would just fire a fatal error at import time. Some other
> compositor might choose to create the dead wl_buffer, send 'failed',
> and do whatever if the client actually uses the dead wl_buffer.
>
> Corollary:
> - 'create_immed' followed by 'wl_display.sync' and wait for the
>   callback is *not* a replacement for the safe 'create', as it may
>   already trigger a fatal error.
>
> The fast way will always be used all in one go, so it would not make a
> difference on when the fatal error would be sent. Once you use
> 'create_immed' at all, you are totally on the implementation-specific
> territory.
>
> In summary, your wayland-protocols patch v2 is very good, but I would
> like you to rephrase the "returned" wording, as requests never return
> anything (the C API functions have a return value, but here we are
> talking about the protocol). To return something implies an event
> delivering it, which would be a roundtrip.
>
> I would also like the wording to more closely match what I wrote above.
> I think it would be important to say clearly that the response to
> 'create_immed' failing is either a fatal protocol error or 'failed'
> event, and it is implementation-specific. That should make it pretty
> clear that 'create_immed' is only safe if you know the compositor and
> the platform.
>
> How's that? Daniel?

Right, thanks for the write-up: that accurately captures my thoughts as well.

Cheers,
Daniel