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

[Pekka Paalanen]

On Mon, 30 Jan 2017 16:00:06 +0530
Varad Gautam <varadgautam at gmail.com> wrote:

> Hi Pekka,
> 
> On Fri, Jan 27, 2017 at 6:32 PM, Pekka Paalanen
> <pekka.paalanen at collabora.co.uk> wrote:
> > On Mon, 21 Nov 2016 21:27:03 +0000
> > Daniel Stone <daniel at fooishbar.org> wrote:
> >  
> >> Hi Varad,
> >>
> >> On 11 November 2016 at 11:40, Varad Gautam <varadgautam at gmail.com> wrote:  
> >> > handle create_immed() dmabuf import requests and support
> >> > zwp_linux_dmabuf_v1_interface version 2.  
> >>
> >> Same caveat about holding off on merging applies, but these two patches are:
> >> Reviewed-by: Daniel Stone <daniels at collabora.com>  
> >
> > Hi,
> >
> > yeah, the Weston patch v1 is probably good, considering my review on
> > the protocol patch v2 essentially asking to go back to the v1 with just
> > some wording added.
> >
> > However, it would be good to check and ensure what happens in Weston
> > when a client uses a wl_buffer that has failed the dmabuf import. I
> > suspect you would need to add some checks somewhere depending on what
> > Weston will do.
> >
> > I think we pretty much agreed that Weston will just send a fatal error
> > to the client if it commits a failed wl_buffer.
> >
> > How to recognize a failed wl_buffer then... maybe NULL user data on the
> > wl_resource? Then there will be places that need to check for NULL: the
> > commit machinery and destructor come to mind.
> >
> > Whatever Weston does, it cannot avoid creating the wl_resource. It must
> > be created, if the protocol XML says a new object is created. And for
> > 'create_immed' request, a new object is created. This is probably
> > overlooked in patch v1.  
> 
> 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.
> 
> [1] https://patchwork.freedesktop.org/patch/133966/

Hi,

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?


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170130/dc6c7792/attachment.sig>