Re: Introduction and updates from NVIDIA

[Andy Ritger]

On Tue, Mar 22, 2016 at 09:52:21PM +, Daniel Stone wrote:
> Hi,
> 
> On 22 March 2016 at 21:43, Daniel Vetter  wrote:
> > On Tue, Mar 22, 2016 at 01:49:59PM +, Daniel Stone wrote:
[...]
> >> I think it's been good to have this series to push the discussion
> >> further in more concrete terms, but unfortunately I have to say that
> >> I'm even less convinced now than I have ever been. Sorry.
> >
> > Well the thing that irks me is that this isn't aiming to build a common
> > platform. There's definitely issues with gbm/gralloc+kms+egl in upstream
> > repos, and vendors have hacked around those in all kinds of horrible ways.
> > But trying to fix this mess with yet another vendor-private solution just
> > doesn't help. Instead we need to fix what is there, for everyone, instead
> > of fragmenting more.
> 
> Agreed. One of the things I've been incredibly happy with is how our
> platform has managed to stay completely generic and vendor-neutral so
> far, and I'd love to preserve that.

I don't think you'll find any disagreement to that from NVIDIA, either.

I apologize if the EGLStreams proposal gave the impression of a
vendor-private solution.  That wasn't the intent.  The EGLStream family
of extensions are, after all, an open specification that any EGL vendor
can implement.  If there are aspects of any of these EGL extensions that
seem useful, I'd hope that Mesa would we willing to adopt them.

We (NVIDIA) clearly think EGLStreams is a good direction for expressing
buffer sharing semantics.  In our ideal world, everyone would implement
these extensions and Wayland compositors would migrate to using them as
the generic vendor-neutral mechanism for buffer sharing :)

But, I'm also happy discuss ways to incrementally improve gbm.  I tried
to address Daniel (Stone)'s questions about NVIDIA's gbm concerns.  For this:

> There's definitely issues with gbm/gralloc+kms+egl in upstream
> repos, and vendors have hacked around those in all kinds of horrible ways.

Some examples were given earlier in this thread.  What are some of the
other horrible hacks drivers have had to use today with gbm?

Thanks,
- Andy

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Introduction and updates from NVIDIA

[Andy Ritger]

Thanks for the thorough responses, Daniel.

On Tue, Mar 22, 2016 at 01:49:59PM +, Daniel Stone wrote:
> Hi Miguel,
> 
> On 21 March 2016 at 16:28, Miguel Angel Vico  wrote:
> > First of all, I'd like to introduce myself to the Wayland community: My
> > name is Miguel A. Vico, and I've been working as a Software Engineer
> > for NVIDIA for some time now, more specifically, in the Linux drivers
> > team. Although I've never spoken before, I've been lately following the
> > amazing work that you all have been doing here.
> 
> Welcome!
> 
> I'm sorry I don't have some better news for you, but Andy and Aaron
> can tell you it's not personal: this has been going on for years.

Yes, we expected this would be somewhat controversial.  I appreciate you
looking at the patch series seriously, Daniel, and especially trying to
drill into the crux of the gbm concerns.

> > In order to make the Weston DRM compositor work with our drivers, we
> > have used EGLDevice, EGLOutput, and EGLStream objects.
> 
> This is ... unfortunate. To echo what Daniel Vetter said, on the whole
> these modesetting-in-EGL extensions are not something which have that
> wide support, or even implementation. That being said, it's
> interesting to have an implementation, because it has helped shape my
> feelings and arguments a little, into something more concrete
> 
> > For those not familiar with this set of EGL structures, here I try to
> > summarize the most important part of them, and how would they fit in
> > the current Weston DRM compositor design:
> >
> > EGLDevice provides means to enumerate native devices, and then
> > create an EGL display connection from them.
> 
> This is generically useful: we would like to extend
> eglGetPlatformDisplay to take an attrib naming an EGLDevice, which we
> could then use with platform_gbm (to select GPU and scanout device
> separately, either for multi-GPU systems or also for SoCs with
> discrete GPU/dispc setups) as well as platform_wayland and co.
> 
> > Similarly, EGLOutput will provide means to access different
> > portions of display control hardware associated with an EGLDevice.
> >
> > For instance, EGLOutputLayer represents a portion of display
> > control hardware that accepts an image as input and processes it
> > for presentation on a display device.
> 
> I still struggle to see the value of what is essentially an
> abstraction over KMS, but oh well.

The intent wasn't to abstract all of KMS, just the surface presentation
aspect where EGL and KMS intersect.  Besides the other points below,
an additional motivation for abstraction is to allow EGL to work with
the native modesetting APIs on other platforms (e.g., OpenWF on QNX).

> > EGLStream implements a mechanism to communicate frame producers and
> > frame consumers. By attaching an EGLOutputLayer consumer to a
> > stream, a producer will be able to present frames on a display
> > device.
> 
> This is understating things quite a bit, I think. On the
> Wayland-client side, it's a pretty big change from the EGLSurface
> model, particularly if you use the default mailbox mode (see comments
> on patch 4/7 as to how this breaks real-world setups, AFAICT).

This shouldn't have a change on the client side: libnvidia-wayland-egl.so
abstracts away the DRM buffer versus EGLStream differences.  These patches
have no effect on the behavior of clients, barring bugs.

> On the Wayland-compositor side, it's two _huge_ changes.
> 
> Firstly, again looking at the case where a Wayland client is a stream
> producer and the Wayland compositor is a consumer, we move from a
> model where references to individual buffers are explicitly passed
> through the Wayland protocol, to where those buffers merely carry a
> reference to a stream. Again, as stated in the review of 4/7, that
> looks like it has the potential to break some actual real-world cases,
> and I have no idea how to solve it, other than banning mailbox mode,
> which would seem to mostly defeat the point of Streams (more on that
> below).

Streams are just a transport for frames.  The client still explicitly
communicates when a frame is delivered through the stream via wayland
protocol, and the compositor controls when it grabs a new frame, via
eglStreamConsumerAcquireKHR().  Unless there are bugs in the patches,
the flow of buffers is still explicit and fully under the wayland protocol
and compositor's control.

Also, mailbox mode versus FIFO mode should essentially equate to Vsync
off versus Vsync on, respectively.  It shouldn't have anything to do
with the benefits of streams, but mailbox mode is a nice feature for
benchmarking games/simulations or naively displaying your latest &
greatest content without tearing.

> Secondly, looking at the compositor-drm case, the use of the dumb
> buffer to display undefined content as a dummy modeset really makes me
> uneasy,

Yes, the use of dumb buffer in this patch series is a kludge.  If 

Re: [PATCH 7/7] compositor-drm: Add support for EGLDevice+EGLOutput

[Miguel Angel Vico]

Hi Daniel,

On Tue, 22 Mar 2016 11:20:47 +
Daniel Stone  wrote:

> Hi,
> 
> On 21 March 2016 at 16:41, Miguel A. Vico 
> wrote:
> > As previously stated, EGLDevice and EGLOutput will provide means
> > to access native device objects and different portions of display
> > control hardware respectively.
> >
> > Whenever EGL_EXT_device_drm extension is present, EGLDevice can
> > be used to enumerate and access DRM KMS devices, and EGLOutputLayer
> > to enumerate and access DRM KMS crtcs and planes.  
> 
> EGLDevice isn't enumerating devices though: you still use udev to
> enumerate DRM devices, and then walk the list of EGLDevices to find
> the device you've already settled on. EGL_DRM_MASTER_FD_EXT then gets
> used to inject an open connection back in to replace the device you've
> already opened. Messy. :\

You're right. I tried to keep compositor-drm as unchanged as possible.

EGLDevice itself doesn't open any connection with the device though.
It's not until EGLDisplay creation that the connection is opened.
EGL_DRM_MASTER_FD_EXT let's us provide EGL with a master FD so certain
operations requiring of master permission can be performed.

We could do the other way around as well: enumerate devices with
EGLDevice, pick the desired one and use it's filename to open a drm
device, then create an EGL display connection.

> 
> > By using EGLStreams and attaching an EGLOutputLayer consumer
> > (representing a DRM KMS crtc or plane) to it, compositor-drm can
> > produce final composition frames and present them on a DRM device.  
> 
> It's gl-renderer which produces the frames, really.

Correct.

> 
> > This change adds required logic to support presentation through
> > EGLDevice+EGLOutput+EGLStream. Whether GBM or EGLDevice should be
> > used can be controlled by --use-egldevice backend argument.  
> 
> Should this not be automatic, i.e. use gbm if platform_gbm is present
> and EGLDevice if gbm isn't present but the other extensions are?

I wanted to be symmetrical to what's currently done with --use-pixman.
If we ever have a driver supporting both GBM and EGLStream
implementations, we might want to be able to switch between them at
runtime.

> 
> > @@ -531,17 +540,21 @@ drm_output_render_gl(struct drm_output
> > *output, pixman_region32_t *damage)
> > output->base.compositor->renderer->repaint_output(>base,
> > damage);
> >
> > -   bo = gbm_surface_lock_front_buffer(output->gbm_surface);
> > -   if (!bo) {
> > -   weston_log("failed to lock front buffer: %m\n");
> > -   return;
> > -   }
> > +   if (b->use_egldevice)
> > +   output->next = output->dumb[0];  
> 
> Huh?
> 
> I'm assuming you use two (never actually used) dumb BOs as
> output->{current,next} to fit in with compositor-drm's bookkeeping,
> whilst Streams handles the actual buffer management for you, right?
> That seems like quite a hack.

Right.

> 
> >  /* When initializing EGL, if the preferred buffer format isn't
> > available
> >   * we may be able to susbstitute an ARGB format for an XRGB one.  
> 
> This seems like it doesn't apply
>  changed the logic to al

What do you mean?

> > @@ -1594,38 +1642,59 @@ fallback_format_for(uint32_t format)
> >  static int
> >  drm_backend_create_gl_renderer(struct drm_backend *b)
> >  {
> > -   EGLint format[3] = {
> > +   EGLint platform_attribs[] = {
> > +   EGL_DRM_MASTER_FD_EXT, b->drm.fd,
> > +   EGL_NONE
> > +   };  
> 
> Seems like this should be called device_platform_attribs, since
> they're only used in the EGLDevice case, not gbm.

Will apply your suggestion.

> 
> > +   EGLint format[] = {
> > b->gbm_format,
> > fallback_format_for(b->gbm_format),
> > -   0,
> > +   0  
> 
> Unrelated changes to the format array here.

True, will undo.

> 
> > +   if (b->use_egldevice) {
> > +   b->egldevice = create_egldevice(b->drm.filename);
> > +   if (b->egldevice == EGL_NO_DEVICE_EXT)
> > +   return -1;
> > +   } else {
> > +   b->gbm = create_gbm_device(b->drm.fd);
> > +   if (!b->gbm)
> > +   return -1;
> > +   }
> > +
> > if (drm_backend_create_gl_renderer(b) < 0) {
> > -   gbm_device_destroy(b->gbm);
> > +   if (b->gbm)
> > +   gbm_device_destroy(b->gbm);  
> 
> Shouldn't the egldevice be destroyed here too?

EGLDevice is only used for enumeration purposes, no connection with
the native device is actually opened, so no need for destruction
whatsoever.

I see now that "create_egldevice()" might be a misleading name for that
function. Probably something like "find_egldevice()" would be better.

> 
> > +   if (b->use_egldevice) {
> > +   int w = output->base.current_mode->width;
> > +   int h = output->base.current_mode->height;
> >
> > - 

Re: [PATCH 5/7] compositor-drm: Gracefully handle vblank and flip invalid timestamps

[Miguel Angel Vico]

Hi,

On Tue, 22 Mar 2016 09:30:51 +
Daniel Stone  wrote:

> Hi,
> 
> On 22 March 2016 at 08:50, Daniel Vetter  wrote:
> > On Tue, Mar 22, 2016 at 08:47:12AM +, Daniel Stone wrote:  
> >> This is odd. Instant queries failing is fine, but in which
> >> circumstances can you fail to return a valid timestamp in a
> >> pageflip event ... ? I don't know of any other kernel driver which
> >> does this - at the very least, if you don't have a hardware
> >> timestamp, then you log the time in your kernel driver which
> >> handles the hardware event (e.g. softirq handler), and use that
> >> instead.  
> >
> > nouveau bugs iirc. Yes, should never happen, it's a bug in the
> > kernel driver that iirc Mario recently tracked down. nouveau
> > somehow started the vblank machinery before the pipe was up, or
> > something like that.
> >
> > So yeah happens, I'm not sure we want to let kernel drivers get
> > away with plain broken behavior forever. I'm leaning towards nack,
> > pls fix the kernel driver.  
> 
> Yes, particularly as no-one has reported it on any other drivers. I
> don't think it's behaviour we really want to support.
> 
> > And maybe make sure the corresponding igt pass ;-)  
> 
> For reference, igt here is intel-gpu-tools, which despite its name is
> actually a generic KMS test/conformance suite. Most of the tests are
> still Intel-specific, but we are actively working on making them
> generic - I was actually in the middle of reviewing a patchset to make
> a rather larger chunk of it generic when I got distracted by this
> thread. ;) It's a great way to find out ways in which your driver
> breaks userspace expectations of a 'well-behaved' KMS driver early.

Thanks guys! We'll fix this in our driver and remove the workaround in
weston.

> 
> Cheers,
> Daniel


-- 
Miguel

NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 4/7] gl-renderer: Add support for EGLDevice+EGLOutput

[Miguel Angel Vico]

Hi Daniel,

On Tue, 22 Mar 2016 10:53:38 +
Daniel Stone  wrote:

> Hi Miguel,
> 
> On 21 March 2016 at 16:37, Miguel A. Vico 
> wrote:
> > EGLDevice provides means to enumerate native devices, and then
> > create an EGL display connection from them.
> >
> > Similarly, EGLOutput will provide means to access different
> > portions of display control hardware associated with an EGLDevice.
> >
> > For instance, EGLOutputLayer represents a portion of display
> > control hardware that accepts an image as input and processes it
> > for presentation on a display device.
> >
> > EGLStream implements a mechanism to communicate frame producers and
> > frame consumers. By attaching an EGLOutputLayer consumer to a
> > stream, a producer will be able to present frames on a display
> > device.
> >
> > Thus, a compositor could produce frames and feed them to an
> > EGLOutputLayer through an EGLStream for presentation on a display
> > device.
> >
> > In a similar way, by attaching a GLTexture consumer to a stream, a
> > producer (wayland client) could feed frames to a texture, which in
> > turn can be used by a compositor to prepare the final frame to be
> > presented.
> >
> > This change adds required logic to support presentation approach
> > described above.
> >
> > Note that some unpublished EGL extensions were needed:
> >
> >  - EGL_NV_stream_attrib:
> >
> > https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_stream_attrib.txt
> >
> >  - EGL_EXT_stream_acquire_mode:
> >
> > https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_EXT_stream_acquire_mode.txt
> >
> >  - EGL_NV_output_drm_flip_event:
> >
> > https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_output_drm_flip_event.txt
> >
> > Also, in order to allow wl_buffers to be bound to EGLStreams, we
> > kludged eglQueryWaylandBufferWL(EGL_WAYLAND_BUFFER_WL) to return
> > the stream file descriptor.  
> 
> Ugh, that's pretty unpleasant.
> 
> > We think the proper way to handle this should be:
> >
> >  - Update WL_bind_wayland_display such that
> > eglQueryWaylandBufferWL() accepts a new attribute
> > EGL_WAYLAND_BUFFER_TYPE_WL, returning
> > EGL_WAYLAND_BUFFER_EGLIMAGE_WL for the non-stream case.
> >
> >  - Add a new WL_wayland_buffer_eglstream extension, which would
> > define EGL_WAYLAND_BUFFER_EGLSTREAM_WL as a return value for
> >EGL_WAYLAND_BUFFER_TYPE_WL, and yet another attribute
> >EGL_WAYLAND_BUFFER_EGLSTREAM_FD_WL to query the stream file
> > descriptor.  
> 
> I disagree quite strongly with this, since it has unpleasant
> compatibility implications. I'd rather see a new EGL_WAYLAND_STREAM_WL
> token or similar, since a stream is by definition not a buffer.

IIUC, EGL_WAYLAND_BUFFER_WL is actually used to create an EGLImage from
a wl_buffer. I don't see how adding a new EGL_WAYLAND_STREAM_WL token
would help here.

We could instead add a new eglQueryWaylandStreamWL() function to query
attributes of a stream, and leave the WL_bind_wayland_display
extension untouched.

> 
> > @@ -84,6 +85,8 @@ struct gl_output_state {
> > struct gl_border_image borders[4];
> > enum gl_border_status border_status;
> >
> > +   EGLStreamKHR egl_stream;
> > +
> > struct weston_matrix output_matrix;
> >  };
> >
> > @@ -159,6 +162,9 @@ struct gl_surface_state {
> > int height; /* in pixels */
> > int y_inverted;
> >
> > +   EGLStreamKHR egl_stream;
> > +   bool stream_frame_acquired;
> > +
> > struct weston_surface *surface;
> >
> > struct wl_listener surface_destroy_listener;
> > @@ -202,6 +208,36 @@ struct gl_renderer {
> >
> > int has_configless_context;
> >
> > +   PFNEGLCREATESTREAMKHRPROC create_stream;
> > +   PFNEGLDESTROYSTREAMKHRPROC destroy_stream;
> > +   PFNEGLQUERYSTREAMKHRPROC query_stream;
> > +   int has_egl_stream;
> > +
> > +   PFNEGLCREATESTREAMPRODUCERSURFACEKHRPROC
> > create_stream_producer_surface;
> > +   int has_egl_stream_producer_eglsurface;
> > +
> > +   PFNEGLSTREAMCONSUMEROUTPUTEXTPROC stream_consumer_output;
> > +   int has_egl_stream_consumer_egloutput;
> > +
> > +   PFNEGLSTREAMCONSUMERACQUIREKHRPROC stream_consumer_acquire;
> > +#ifdef EGL_EXT_stream_acquire_mode
> > +   PFNEGLSTREAMCONSUMERACQUIREATTRIBEXTPROC
> > stream_consumer_acquire_attrib; +#endif
> > +   int has_egl_stream_acquire_mode;
> > +
> > +   int has_egl_output_drm;
> > +   int has_egl_output_drm_flip_event;
> > +
> > +   PFNEGLGETOUTPUTLAYERSEXTPROC get_output_layers;
> > +   PFNEGLQUERYOUTPUTLAYERATTRIBEXTPROC
> > query_output_layer_attrib;
> > +   int has_egl_output_base;
> > +
> > +   PFNEGLCREATESTREAMFROMFILEDESCRIPTORKHRPROC
> > create_stream_from_file_descriptor;
> > +   int has_egl_stream_cross_process_fd;
> > +
> > +   

Re: [PATCH 2/7] gl-renderer: Refactor gl_renderer_output_window_create()

[Miguel Angel Vico]

Hi Daniel,

On Tue, 22 Mar 2016 09:02:55 +
Daniel Stone  wrote:

> Hi Miguel,
> 
> On 21 March 2016 at 16:37, Miguel A. Vico 
> wrote:
> > +   egl_surface = gl_renderer_create_window_surface(gr,
> > +
> > window_for_legacy,
> > +
> > window_for_platform,
> > +
> > config_attribs,
> > +   visual_id,
> > n_ids); +
> > +   ret = gl_renderer_output_create(output, egl_surface);
> > +   if (ret < 0 && egl_surface != EGL_NO_SURFACE)
> > +   eglDestroySurface(gr->egl_display, egl_surface);  
> 
> I'm not a huge fan of the error-handling here: I'd rather see an
> instant return -1 from this function if we can't create an EGLSurface,
> rather than relying on gl_renderer_output_create to check and handle
> it for us.

Heh, yeah, I'll change it.

Thanks.

> 
> Otherwise I guess this looks OK.
> 
> Cheers,
> Daniel


-- 
Miguel

NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 1/7] gl-renderer: Renaming of things and minor improvements

[Miguel Angel Vico]

Hi Daniel,

On Tue, 22 Mar 2016 09:06:25 +
Daniel Stone  wrote:

> Hi Miguel,
> 
> On 21 March 2016 at 16:37, Miguel A. Vico 
> wrote:
> > In preparation for follow-on changes to support frame presentation
> > through EGLDevice+EGLOutput, this change includes the following:
> >   -Rename gl_renderer_output_create to
> > gl_renderer_output_window_create -Add  argument
> > to gl_renderer_create -Rename  argument for
> > gl_renderer_create() and gl_renderer_output_window_create() to
> >  -Accept non-NULL empty  arrays (n_ids
> > == 0) both in gl_renderer_create() and
> > gl_renderer_output_window_create()  
> 
> It would be nice to split these up into discrete changes: one per
> bullet point is a pretty good guideline. While you're at it,
> gl_renderer->create() could definitely be renamed to something more
> usefully specific, e.g. gl_render->display_create.

Consider it done.

> 
> > diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> > index cb08344..0c55e0b 100644
> > --- a/src/gl-renderer.c
> > +++ b/src/gl-renderer.c
> > @@ -2493,7 +2493,7 @@ egl_choose_config(struct gl_renderer *gr,
> > const EGLint *attribs, goto out;
> > }
> >
> > -   if (!visual_id)
> > +   if (!visual_id || n_ids == 0)
> > config_index = 0;
> >
> > for (i = 0; config_index == -1 && i < n_ids; i++)  
> 
> This bit I'm a bit confused about though. How do you end up in this
> situation?

An early version of the patches was passing a non-null visual_id array
with n_ids == 0 for the streams case, which I think is valid. I
thought it wouldn't hurt to also check for n_ids != 0 there and avoid
possible bugs in the future.

> 
> The rest look good to me, once split into separate patches.
> 
> Cheers,
> Daniel


-- 
Miguel

NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Introduction and updates from NVIDIA

[Daniel Stone]

Hi,

On 22 March 2016 at 21:43, Daniel Vetter  wrote:
> On Tue, Mar 22, 2016 at 01:49:59PM +, Daniel Stone wrote:
>> 'We don't know when to schedule decompression, because there's no
>> explicit barrier' - completely untrue. eglSwapBuffers is that barrier.
>> For example, in Freescale i.MX6, the Vivante GPU and Freescale IPU
>> (display controller) do not share a single common format between GPU
>> render targets and IPU scanout sources, so require a mandatory
>> detiling pass in between render and display. These work just fine with
>> gbm with that pass scheduled by eglSwapBuffers. This to me seems
>> completely explicit, unless there was something else you were meaning
>> ... ?
>
> There's display engines which can directly scan out buffers compressed by
> the render engine. It's awesome, except randomly limited, so you need a
> communication backchannel from your display driver all the way to your
> buffer allocator thing on the client side. And depending upon luck you
> really can't tell who should do the decompress past for most optimal
> result upfront.
>
> I think on android the most common way to do that is to attach arbitrary
> metadata with a hand-rolled ioctl to dma-buf fds, which is ofc horrible.
> Imo the right way is to create a real platform and start to standardize
> some of this stuff (fb modifier) more, so that we can pass it from kms to
> gbm, then to compositor clients through either a generic transport of
> private extensions. Or maybe we can mostly hide all that.

Right, at least with some (AFBC), just the buffer data + FB modifier
completely describes what you need to scan out transparently. Though
this is not the case for Intel and Tegra.

>> 'Width, height, pitch and format aren't enough information' - this is
>> true, but not necessarily relevant. I'm not sure what the source of
>> this actually is: is it the gbm_bo_get_*() APIs? If so, yes, they need
>> to be extended with a gbm_bo_get_modifier() call, which would allow
>> you to get the DRM format modifier to describe tiling/compression/et
>> al (as well as perhaps being extended to allow you to extract multiple
>> buffers/planes, e.g. to attach auxiliary compression buffers). If it's
>> not gbm, what actually is it? The only other place I can think of
>> (suggested by Pekka, I think) was the wl_drm protocol, which it should
>> be stressed is a) not required in any way by Wayland, b) not a
>> published/public protocol, c) not a stable protocol. wl_drm just
>> happens to be the way that Mesa shares buffers, just as wl_viv is how
>> Vivante's proprietary driver shares buffers, and mali_buffer_sharing
>> is how the Mali driver does it. Since the server side is bound by
>> eglBindWaylandDisplayWL and the client side is also only used through
>> EGL, there is _no_ requirement for you to also implement wl_drm. As it
>> is a hidden private Mesa protocol, there is also no requirement for
>> the protocol to remain stable.
>
> So I've what our own android folks all transport, and I think most of it
> we can transport with the current addfb2.1 kms metadata. And we could even
> add hints that kms atomic returns if a plane doesn't work with the most
> preferred format that would just work in this config. Thus far I've
> stumbled over 2 cases:
> - compression formats that can't be easily described in addfb2.1 because
>   they allocate a side buffer in some fancy special memory. The solution
>   for that that was discussed at xdc2014 was to use a dma-buf to wrap that
>   up, and then use as aux buffer (there's patches floating for that) with
>   normal addfb2.1.

Indeed, although sadly the current Intel patches go in the other
direction and use a driver-private plane property to describe the
current compression status. :( Hopefully the Tegra/Nouveau people are
able to prepare something which is usable from generic userspace.

>> 'EGLStreams is the direction taken in Vulkan' - I would argue not. IMO
>> the explicit buffer management on the client side does not parallel
>> EGLStreams, and notably there is no equivalent consumer interface
>> offered on the server side, but instead the individual-buffer-driven
>> approach is taken. It's true that VK_WSI_display_swapchain does exist
>> and does match the EGLStreams model fairly closely, but also that it
>> does not have universal implementation: the Intel 'anv' Mesa-based
>> driver does not implement display_swapchain, instead having an
>> interface to export a VkImage as a dmabuf. It's true that the latter
>> is not optimal (it lacks the explicit targeting required to determine
>> the most optimal tiling/compression strategy), but OTOH it is
>> precedent for explicitly avoiding the
>> VK_WSI_display_swapchain/EGLStreams model for Vulkan on KMS, just as
>> GBM avoids it for EGL on KMS.
>
> I'm not sure a swapchain/stream is good enough, since the trouble really
> starts when you have tons of hw planes and changing configurations.
> Looking at individual streams instead of the 

Re: Introduction and updates from NVIDIA

[Daniel Vetter]

On Tue, Mar 22, 2016 at 01:49:59PM +, Daniel Stone wrote:
> On 21 March 2016 at 16:28, Miguel Angel Vico  wrote:
> I'd like to look at the elephant in the room, which is why you're
> using this in the first place (aside from general NVIDIA enthusiasm
> for encapsulating everything within EGL Streams/Output/Device/etc,
> dating back many years). Andy/Aaron, you've said that you found GBM to
> be inadequate, and I'd like to find out explicitly how. Through a few
> snippets of IRC and NVIDIA devtalk, so far I can see:
> 
> 'We can't choose an optimal rendering configuration, because we don't
> know how it's going to be used' - (almost completely) untrue. The FD
> you pass to gbm_device_create is that of the KMS device, a gbm_surface
> contains information as to how the plane (primary or overlay) will be
> configured, and an EGLDisplay lets you tie the rendering and scanout
> devices together. What more information do you need? It's true that we
> don't have a way to select individual rendering devices at the moment,
> but as said earlier, passing an EGLDevice as an attrib to
> GetPlatformDisplay would resolve that, as you would have the render
> device identified by the EGLDevice and the scanout device identified
> by the gbm_device. At that point, you have the full pipeline and can
> determine the optimal configuration.
> 
> 'We don't know when to schedule decompression, because there's no
> explicit barrier' - completely untrue. eglSwapBuffers is that barrier.
> For example, in Freescale i.MX6, the Vivante GPU and Freescale IPU
> (display controller) do not share a single common format between GPU
> render targets and IPU scanout sources, so require a mandatory
> detiling pass in between render and display. These work just fine with
> gbm with that pass scheduled by eglSwapBuffers. This to me seems
> completely explicit, unless there was something else you were meaning
> ... ?

There's display engines which can directly scan out buffers compressed by
the render engine. It's awesome, except randomly limited, so you need a
communication backchannel from your display driver all the way to your
buffer allocator thing on the client side. And depending upon luck you
really can't tell who should do the decompress past for most optimal
result upfront.

I think on android the most common way to do that is to attach arbitrary
metadata with a hand-rolled ioctl to dma-buf fds, which is ofc horrible.
Imo the right way is to create a real platform and start to standardize
some of this stuff (fb modifier) more, so that we can pass it from kms to
gbm, then to compositor clients through either a generic transport of
private extensions. Or maybe we can mostly hide all that.

> 'Width, height, pitch and format aren't enough information' - this is
> true, but not necessarily relevant. I'm not sure what the source of
> this actually is: is it the gbm_bo_get_*() APIs? If so, yes, they need
> to be extended with a gbm_bo_get_modifier() call, which would allow
> you to get the DRM format modifier to describe tiling/compression/et
> al (as well as perhaps being extended to allow you to extract multiple
> buffers/planes, e.g. to attach auxiliary compression buffers). If it's
> not gbm, what actually is it? The only other place I can think of
> (suggested by Pekka, I think) was the wl_drm protocol, which it should
> be stressed is a) not required in any way by Wayland, b) not a
> published/public protocol, c) not a stable protocol. wl_drm just
> happens to be the way that Mesa shares buffers, just as wl_viv is how
> Vivante's proprietary driver shares buffers, and mali_buffer_sharing
> is how the Mali driver does it. Since the server side is bound by
> eglBindWaylandDisplayWL and the client side is also only used through
> EGL, there is _no_ requirement for you to also implement wl_drm. As it
> is a hidden private Mesa protocol, there is also no requirement for
> the protocol to remain stable.

So I've what our own android folks all transport, and I think most of it
we can transport with the current addfb2.1 kms metadata. And we could even
add hints that kms atomic returns if a plane doesn't work with the most
preferred format that would just work in this config. Thus far I've
stumbled over 2 cases:
- compression formats that can't be easily described in addfb2.1 because
  they allocate a side buffer in some fancy special memory. The solution
  for that that was discussed at xdc2014 was to use a dma-buf to wrap that
  up, and then use as aux buffer (there's patches floating for that) with
  normal addfb2.1.
- content protection. Can't talk about this, but worst case it can all be
  captured in special-purpose buffers too I think.

> 'EGLStreams is the direction taken in Vulkan' - I would argue not. IMO
> the explicit buffer management on the client side does not parallel
> EGLStreams, and notably there is no equivalent consumer interface
> offered on the server side, but instead the individual-buffer-driven

Re: [PATCH 4/7] gl-renderer: Add support for EGLDevice+EGLOutput

[Giulio Camuffo]

2016-03-21 18:37 GMT+02:00 Miguel A. Vico :

>  static void
> +gl_renderer_attach_egl_fd_texture(struct weston_surface *es,
> +  struct weston_buffer *buffer,
> +  EGLNativeFileDescriptorKHR streamFd)
> +{
> +   struct weston_compositor *ec = es->compositor;
> +   struct gl_renderer *gr = get_renderer(ec);
> +   struct gl_surface_state *gs = get_surface_state(es);
> +
> +   /* If an invalid streamFd is provided, either there was an error 
> creating
> +* the buffer or this buffer is already attached. Either case, there's
> +* nothing to be done */
> +   if (streamFd < 0)
> +   return;
> +
> +   /* Clean up current stream resources, if needed */
> +   if (gs->egl_stream != EGL_NO_STREAM_KHR) {
> +   gr->destroy_stream(gr->egl_display, gs->egl_stream);
> +   gs->egl_stream = EGL_NO_STREAM_KHR;
> +   gs->stream_frame_acquired = false;
> +   }
> +
> +   gs->egl_stream =
> +   gr->create_stream_from_file_descriptor(gr->egl_display, 
> streamFd);
> +   close(streamFd);
> +
> +   if (gs->egl_stream == EGL_NO_STREAM_KHR) {
> +   weston_log("failed to create egl stream\n");
> +   goto err_attach_egl_fd_base;
> +   }
> +
> +   gs->shader = >texture_shader_egl_external;
> +   gs->target = GL_TEXTURE_EXTERNAL_OES;
> +
> +   glActiveTexture(GL_TEXTURE0);
> +   ensure_textures(gs, 1);
> +   glBindTexture(gs->target, gs->textures[0]);
> +
> +   if (gr->stream_consumer_gltexture(gr->egl_display, gs->egl_stream) != 
> EGL_TRUE) {

This bothers me quite a lot... my issue with EGLStream is that the
spec says that the consumer must be created before the producer. So
this leaves me wondering, if you create the consumer at attach time,
when is the producer created? I assume it's not created when the
client creates the surface, becase the attach would be late. I guess
then that you're creating the producer some time after the attach,
probably by roundtripping just after the attach/commit?
That is all fine and dandy on a single threaded compositor like
weston, that always has a current context when it receives the buffer,
but what should a compositor with a render thread do? It likely
doesn't have a current context when it processes the attach/commit, or
if it does it's on the wrong thread.
This is a huge problem imho, unless i'm missing something?


Thanks,
Giulio


> +   weston_log("failed to set stream consumer\n");
> +   goto err_attach_egl_fd_stream;
> +   }
> +
> +   buffer->legacy_buffer = (void *)buffer->resource;
> +   gr->query_buffer(gr->egl_display, buffer->legacy_buffer,
> +EGL_WIDTH, >width);
> +   gr->query_buffer(gr->egl_display, buffer->legacy_buffer,
> +EGL_HEIGHT, >height);
> +   buffer->y_inverted = 0;
> +
> +   gs->pitch = buffer->width;
> +   gs->height = buffer->height;
> +   gs->buffer_type = BUFFER_TYPE_EGL;
> +   gs->y_inverted = buffer->y_inverted;
> +
> +   return;
> +
> +err_attach_egl_fd_stream:
> +   gr->destroy_stream(gr->egl_display, gs->egl_stream);
> +   gs->egl_stream = EGL_NO_STREAM_KHR;
> +
> +err_attach_egl_fd_base:
> +   gl_renderer_print_egl_error_state();
> +   return;
> +}
> +
> +static void
>  gl_renderer_attach(struct weston_surface *es, struct weston_buffer *buffer)
>  {
> struct weston_compositor *ec = es->compositor;
> @@ -1891,6 +2050,7 @@ gl_renderer_attach(struct weston_surface *es, struct 
> weston_buffer *buffer)
> struct gl_surface_state *gs = get_surface_state(es);
> struct wl_shm_buffer *shm_buffer;
> struct linux_dmabuf_buffer *dmabuf;
> +   EGLNativeFileDescriptorKHR streamFd;
> EGLint format;
> int i;
>
> @@ -1906,6 +2066,13 @@ gl_renderer_attach(struct weston_surface *es, struct 
> weston_buffer *buffer)
> gs->num_textures = 0;
> gs->buffer_type = BUFFER_TYPE_NULL;
> gs->y_inverted = 1;
> +
> +   if (gs->egl_stream != EGL_NO_STREAM_KHR) {
> +   gr->destroy_stream(gr->egl_display, gs->egl_stream);
> +   gs->egl_stream = EGL_NO_STREAM_KHR;
> +   gs->stream_frame_acquired = false;
> +   }
> +
> return;
> }
>
> @@ -1918,7 +2085,11 @@ gl_renderer_attach(struct weston_surface *es, struct 
> weston_buffer *buffer)
> gl_renderer_attach_egl(es, buffer, format);
> else if ((dmabuf = linux_dmabuf_buffer_get(buffer->resource)))
> gl_renderer_attach_dmabuf(es, buffer, dmabuf);
> -   else {
> +   else if (gr->query_buffer(gr->egl_display, (void *) buffer->resource,
> + EGL_WAYLAND_BUFFER_WL, )) {
> +   /* 

[PATCH weston v6] input: use doubles in the interfaces to notify of input events

[Giulio Camuffo]

This patch is a further step in the wl_fixed_t internal sanitization.
It changes the notify_* functions to take doubles instead of wl_fixed_t
but does not change how these are stored in the various input structs
yet, except for weston_pointer_axis_event.
However this already allows to remove all wl_fixed_t usage in places
like the libinput or the x11 backend.

Reviewed-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---

v6: fixed pointer going out of sync with the x11 backend

 desktop-shell/shell.c|  6 +++---
 src/compositor-rdp.c | 12 +++
 src/compositor-wayland.c | 56 +---
 src/compositor-x11.c | 22 +--
 src/compositor.c | 12 +--
 src/compositor.h | 12 +--
 src/input.c  | 19 ++--
 src/libinput-device.c| 26 ++
 src/screen-share.c   |  5 +++--
 9 files changed, 89 insertions(+), 81 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e1fefae..24437d8 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4786,7 +4786,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
uint32_t time,
if (!shsurf)
return;
 
-   shsurf->view->alpha -= wl_fixed_to_double(event->value) * step;
+   shsurf->view->alpha -= event->value * step;
 
if (shsurf->view->alpha > 1.0)
shsurf->view->alpha = 1.0;
@@ -4799,7 +4799,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
uint32_t time,
 
 static void
 do_zoom(struct weston_seat *seat, uint32_t time, uint32_t key, uint32_t axis,
-   wl_fixed_t value)
+   double value)
 {
struct weston_compositor *compositor = seat->compositor;
struct weston_pointer *pointer = weston_seat_get_pointer(seat);
@@ -4823,7 +4823,7 @@ do_zoom(struct weston_seat *seat, uint32_t time, uint32_t 
key, uint32_t axis,
else if (axis == WL_POINTER_AXIS_VERTICAL_SCROLL)
/* For every pixel zoom 20th of a step */
increment = output->zoom.increment *
-   -wl_fixed_to_double(value) / 20.0;
+   -value / 20.0;
else
increment = 0;
 
diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
index e603b76..773b6b5 100644
--- a/src/compositor-rdp.c
+++ b/src/compositor-rdp.c
@@ -942,7 +942,6 @@ static BOOL xf_peer_post_connect(freerdp_peer *client)
 static FREERDP_CB_RET_TYPE
 xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
uint32_t button = 0;
@@ -951,10 +950,8 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
if (flags & PTR_FLAGS_MOVE) {
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
need_frame = true;
}
}
@@ -988,7 +985,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
value = -value;
 
weston_event.axis = WL_POINTER_AXIS_VERTICAL_SCROLL;
-   weston_event.value = 
wl_fixed_from_double(DEFAULT_AXIS_STEP_DISTANCE * value);
+   weston_event.value = DEFAULT_AXIS_STEP_DISTANCE * value;
weston_event.discrete = (int)value;
weston_event.has_discrete = true;
 
@@ -1006,16 +1003,13 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
 static FREERDP_CB_RET_TYPE
 xf_extendedMouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
 
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
}
 
FREERDP_CB_RETURN(TRUE);
diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
index 8a1878c..9d1a251 100644
--- a/src/compositor-wayland.c
+++ b/src/compositor-wayland.c
@@ -1275,11 +1275,15 @@ input_set_cursor(struct wayland_input 

Re: [PATCH weston v5] input: use doubles in the interfaces to notify of input events

[Pekka Paalanen]

On Tue, 22 Mar 2016 16:53:14 +0200
Giulio Camuffo  wrote:

> This patch is a further step in the wl_fixed_t internal sanitization.
> It changes the notify_* functions to take doubles instead of wl_fixed_t
> but does not change how these are stored in the various input structs
> yet, except for weston_pointer_axis_event.
> However this already allows to remove all wl_fixed_t usage in places
> like the libinput or the x11 backend.
> 
> Reviewed-by: Daniel Stone 
> Reviewed-by: Pekka Paalanen 
> ---
> 
> v5: fixed wl_fixed_t usage in desktop-shell's do_zoom()
> 
>  desktop-shell/shell.c|  6 +++---
>  src/compositor-rdp.c | 12 +++
>  src/compositor-wayland.c | 56 
> +---
>  src/compositor-x11.c | 22 +--
>  src/compositor.c | 12 +--
>  src/compositor.h | 12 +--
>  src/input.c  | 15 +++--
>  src/libinput-device.c| 26 ++
>  src/screen-share.c   |  5 +++--
>  9 files changed, 86 insertions(+), 80 deletions(-)
> 

Hi,

as discussed in IRC, I almost pushed this patch after fixing the stray
parenthesis, only to find out it breaks cursor tracking on x11-backend
when the cursor leaves and enters the x11 window. For good measure, you
can (must?) mix zooming with it.


Thanks,
pq


pgpH7NPhdECD3.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 4/5] ivi-shell: harden get_ivi_shell_surface()

[Pekka Paalanen]

From: Pekka Paalanen 

Add more sanity checks to get_ivi_shell_surface() just in case.

If the configure hook is set, we must always have non-NULL
configure_private.

Check the ivi_shell_surface matches the surface.

Signed-off-by: Pekka Paalanen 
---
 ivi-shell/ivi-shell.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index d136f46..c502c74 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -81,10 +81,16 @@ ivi_shell_surface_configure(struct weston_surface *, 
int32_t, int32_t);
 static struct ivi_shell_surface *
 get_ivi_shell_surface(struct weston_surface *surface)
 {
-   if (surface->configure == ivi_shell_surface_configure)
-   return surface->configure_private;
+   struct ivi_shell_surface *shsurf;
+
+   if (surface->configure != ivi_shell_surface_configure)
+   return NULL;
+
+   shsurf = surface->configure_private;
+   assert(shsurf);
+   assert(shsurf->surface == surface);
 
-   return NULL;
+   return shsurf;
 }
 
 void
-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v5] input: use doubles in the interfaces to notify of input events

[Giulio Camuffo]

This patch is a further step in the wl_fixed_t internal sanitization.
It changes the notify_* functions to take doubles instead of wl_fixed_t
but does not change how these are stored in the various input structs
yet, except for weston_pointer_axis_event.
However this already allows to remove all wl_fixed_t usage in places
like the libinput or the x11 backend.

Reviewed-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---

v5: fixed wl_fixed_t usage in desktop-shell's do_zoom()

 desktop-shell/shell.c|  6 +++---
 src/compositor-rdp.c | 12 +++
 src/compositor-wayland.c | 56 +---
 src/compositor-x11.c | 22 +--
 src/compositor.c | 12 +--
 src/compositor.h | 12 +--
 src/input.c  | 15 +++--
 src/libinput-device.c| 26 ++
 src/screen-share.c   |  5 +++--
 9 files changed, 86 insertions(+), 80 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e1fefae..bd67697 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4786,7 +4786,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
uint32_t time,
if (!shsurf)
return;
 
-   shsurf->view->alpha -= wl_fixed_to_double(event->value) * step;
+   shsurf->view->alpha -= event->value * step;
 
if (shsurf->view->alpha > 1.0)
shsurf->view->alpha = 1.0;
@@ -4799,7 +4799,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
uint32_t time,
 
 static void
 do_zoom(struct weston_seat *seat, uint32_t time, uint32_t key, uint32_t axis,
-   wl_fixed_t value)
+   double value)
 {
struct weston_compositor *compositor = seat->compositor;
struct weston_pointer *pointer = weston_seat_get_pointer(seat);
@@ -4823,7 +4823,7 @@ do_zoom(struct weston_seat *seat, uint32_t time, uint32_t 
key, uint32_t axis,
else if (axis == WL_POINTER_AXIS_VERTICAL_SCROLL)
/* For every pixel zoom 20th of a step */
increment = output->zoom.increment *
-   -wl_fixed_to_double(value) / 20.0;
+   -value) / 20.0;
else
increment = 0;
 
diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
index e603b76..773b6b5 100644
--- a/src/compositor-rdp.c
+++ b/src/compositor-rdp.c
@@ -942,7 +942,6 @@ static BOOL xf_peer_post_connect(freerdp_peer *client)
 static FREERDP_CB_RET_TYPE
 xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
uint32_t button = 0;
@@ -951,10 +950,8 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
if (flags & PTR_FLAGS_MOVE) {
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
need_frame = true;
}
}
@@ -988,7 +985,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
value = -value;
 
weston_event.axis = WL_POINTER_AXIS_VERTICAL_SCROLL;
-   weston_event.value = 
wl_fixed_from_double(DEFAULT_AXIS_STEP_DISTANCE * value);
+   weston_event.value = DEFAULT_AXIS_STEP_DISTANCE * value;
weston_event.discrete = (int)value;
weston_event.has_discrete = true;
 
@@ -1006,16 +1003,13 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
 static FREERDP_CB_RET_TYPE
 xf_extendedMouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
 
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
}
 
FREERDP_CB_RETURN(TRUE);
diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
index 8a1878c..9d1a251 100644
--- a/src/compositor-wayland.c
+++ b/src/compositor-wayland.c
@@ -1275,11 +1275,15 @@ input_set_cursor(struct wayland_input *input)

[PATCH weston 3/5] ivi-shell: add sanity check in ivi_shell_surface_configure

[Pekka Paalanen]

From: Pekka Paalanen 

This should not get called unless there is an ivi_shell_surface.

Signed-off-by: Pekka Paalanen 
---
 ivi-shell/ivi-shell.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 9c11f6f..d136f46 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -108,7 +108,11 @@ ivi_shell_surface_configure(struct weston_surface *surface,
 {
struct ivi_shell_surface *ivisurf = get_ivi_shell_surface(surface);
 
-   if (surface->width == 0 || surface->height == 0 || ivisurf == NULL)
+   assert(ivisurf);
+   if (!ivisurf)
+   return;
+
+   if (surface->width == 0 || surface->height == 0)
return;
 
if (ivisurf->width != surface->width ||
-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 5/5] ivi-shell: forward zero size to controller

[Pekka Paalanen]

From: Pekka Paalanen 

When surface size changes to zero, do not ignore it, but forward that
change to the controller. A controller should do what's proper for a
surface that just lost its content.

Signed-off-by: Pekka Paalanen 
---
 ivi-shell/hmi-controller.c | 2 ++
 ivi-shell/ivi-shell.c  | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c
index e9c9ab0..7c96439 100644
--- a/ivi-shell/hmi-controller.c
+++ b/ivi-shell/hmi-controller.c
@@ -660,6 +660,8 @@ set_notification_configure_surface(struct 
ivi_layout_surface *ivisurf,
 */
surface = ivi_layout_interface->surface_get_weston_surface(ivisurf);
if (surface) {
+   /* XXX: needs to handle zero size by unmapping the surface */
+
ivi_layout_interface->surface_set_source_rectangle(
ivisurf, 0, 0, surface->width,
surface->height);
diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index c502c74..fe86c27 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -118,9 +118,6 @@ ivi_shell_surface_configure(struct weston_surface *surface,
if (!ivisurf)
return;
 
-   if (surface->width == 0 || surface->height == 0)
-   return;
-
if (ivisurf->width != surface->width ||
ivisurf->height != surface->height) {
ivisurf->width  = surface->width;
-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 2/5] ivi-shell: add input panel label func

[Pekka Paalanen]

From: Pekka Paalanen 

Copied from desktop-shell/input-panel.c, add a label function for the
input panel.

This patch strictly reduces the difference between input-panel.c and
input-panel-ivi.c.

Signed-off-by: Pekka Paalanen 
---
 ivi-shell/input-panel-ivi.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/ivi-shell/input-panel-ivi.c b/ivi-shell/input-panel-ivi.c
index 721d31c..581b56b 100644
--- a/ivi-shell/input-panel-ivi.c
+++ b/ivi-shell/input-panel-ivi.c
@@ -157,6 +157,12 @@ update_input_panels(struct wl_listener *listener, void 
*data)
memcpy(>text_input.cursor_rectangle, data, 
sizeof(pixman_box32_t));
 }
 
+static int
+input_panel_get_label(struct weston_surface *surface, char *buf, size_t len)
+{
+   return snprintf(buf, len, "input panel");
+}
+
 static void
 input_panel_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
 {
@@ -194,6 +200,7 @@ destroy_input_panel_surface(struct input_panel_surface 
*input_panel_surface)
wl_list_remove(_panel_surface->link);
 
input_panel_surface->surface->configure = NULL;
+   weston_surface_set_label_func(input_panel_surface->surface, NULL);
weston_view_destroy(input_panel_surface->view);
 
free(input_panel_surface);
@@ -235,6 +242,7 @@ create_input_panel_surface(struct ivi_shell *shell,
 
surface->configure = input_panel_configure;
surface->configure_private = input_panel_surface;
+   weston_surface_set_label_func(surface, input_panel_get_label);
 
input_panel_surface->shell = shell;
 
-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/5] ivi-shell: add shell surface labels

[Pekka Paalanen]

From: Pekka Paalanen 

To be used by the Weston timeline feature for identifying surfaces in a
trace. The 'get_label' functionality can also be used by any debugging
code, too.

Signed-off-by: Pekka Paalanen 
---
 ivi-shell/ivi-shell.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 2ff3784..9c11f6f 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -121,6 +121,19 @@ ivi_shell_surface_configure(struct weston_surface *surface,
}
 }
 
+static int
+ivi_shell_surface_get_label(struct weston_surface *surface,
+   char *buf,
+   size_t len)
+{
+   struct ivi_shell_surface *shell_surf = get_ivi_shell_surface(surface);
+
+   if (!shell_surf)
+   return snprintf(buf, len, "unidentified window in ivi-shell");
+
+   return snprintf(buf, len, "ivi-surface %#x", shell_surf->id_surface);
+}
+
 static void
 layout_surface_cleanup(struct ivi_shell_surface *ivisurf)
 {
@@ -131,6 +144,7 @@ layout_surface_cleanup(struct ivi_shell_surface *ivisurf)
 
ivisurf->surface->configure = NULL;
ivisurf->surface->configure_private = NULL;
+   weston_surface_set_label_func(ivisurf->surface, NULL);
ivisurf->surface = NULL;
 
// destroy weston_surface destroy signal.
@@ -262,6 +276,8 @@ application_surface_create(struct wl_client *client,
 
weston_surface->configure = ivi_shell_surface_configure;
weston_surface->configure_private = ivisurf;
+   weston_surface_set_label_func(weston_surface,
+ ivi_shell_surface_get_label);
 
res = wl_resource_create(client, _surface_interface, 1, id);
if (res == NULL) {
-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 0/5] simple ivi-shell fortification

[Pekka Paalanen]

From: Pekka Paalanen 

Hi,

I have written these simple patches as a side-product, and I'm proposing to
merge them upstream. They are mostly just adding sanity checks, except the last
one that is a possible behavioral change.

I'm not sure what the proper response in hmi-controller would be in patch 5,
and there probably is no way to test it readily. Feel free to reject that one
if it's a too lazy patch.


Thanks,
pq

Pekka Paalanen (5):
  ivi-shell: add shell surface labels
  ivi-shell: add input panel label func
  ivi-shell: add sanity check in ivi_shell_surface_configure
  ivi-shell: harden get_ivi_shell_surface()
  ivi-shell: forward zero size to controller

 ivi-shell/hmi-controller.c  |  2 ++
 ivi-shell/input-panel-ivi.c |  8 
 ivi-shell/ivi-shell.c   | 31 +++
 3 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.7.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Introduction and updates from NVIDIA

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:28, Miguel Angel Vico  wrote:
> First of all, I'd like to introduce myself to the Wayland community: My
> name is Miguel A. Vico, and I've been working as a Software Engineer
> for NVIDIA for some time now, more specifically, in the Linux drivers
> team. Although I've never spoken before, I've been lately following the
> amazing work that you all have been doing here.

Welcome!

I'm sorry I don't have some better news for you, but Andy and Aaron
can tell you it's not personal: this has been going on for years.

> In order to make the Weston DRM compositor work with our drivers, we
> have used EGLDevice, EGLOutput, and EGLStream objects.

This is ... unfortunate. To echo what Daniel Vetter said, on the whole
these modesetting-in-EGL extensions are not something which have that
wide support, or even implementation. That being said, it's
interesting to have an implementation, because it has helped shape my
feelings and arguments a little, into something more concrete

> For those not familiar with this set of EGL structures, here I try to
> summarize the most important part of them, and how would they fit in
> the current Weston DRM compositor design:
>
> EGLDevice provides means to enumerate native devices, and then
> create an EGL display connection from them.

This is generically useful: we would like to extend
eglGetPlatformDisplay to take an attrib naming an EGLDevice, which we
could then use with platform_gbm (to select GPU and scanout device
separately, either for multi-GPU systems or also for SoCs with
discrete GPU/dispc setups) as well as platform_wayland and co.

> Similarly, EGLOutput will provide means to access different
> portions of display control hardware associated with an EGLDevice.
>
> For instance, EGLOutputLayer represents a portion of display
> control hardware that accepts an image as input and processes it
> for presentation on a display device.

I still struggle to see the value of what is essentially an
abstraction over KMS, but oh well.

> EGLStream implements a mechanism to communicate frame producers and
> frame consumers. By attaching an EGLOutputLayer consumer to a
> stream, a producer will be able to present frames on a display
> device.

This is understating things quite a bit, I think. On the
Wayland-client side, it's a pretty big change from the EGLSurface
model, particularly if you use the default mailbox mode (see comments
on patch 4/7 as to how this breaks real-world setups, AFAICT). On the
Wayland-compositor side, it's two _huge_ changes.

Firstly, again looking at the case where a Wayland client is a stream
producer and the Wayland compositor is a consumer, we move from a
model where references to individual buffers are explicitly passed
through the Wayland protocol, to where those buffers merely carry a
reference to a stream. Again, as stated in the review of 4/7, that
looks like it has the potential to break some actual real-world cases,
and I have no idea how to solve it, other than banning mailbox mode,
which would seem to mostly defeat the point of Streams (more on that
below).

Secondly, looking at the compositor-drm case, the use of the dumb
buffer to display undefined content as a dummy modeset really makes me
uneasy, again because both gl-renderer and compositor-drm are written
for explicit individual buffer management, rather than streams in +
streams out. I think the combination of the two pushes them long
beyond the point of readability, and I'd encourage you to look at
trying to split those files up, or at least the functions within them.
Attempting to keep both modes in there just looks like a maintenance
nightmare, especially when this streams implementation
(unsurprisingly) has to bypass almost the entire runtime (as opposed
to init-time) functionality of compositor-drm.

Also, I'm not quite sure how you're testing the compositor-as-consumer
mode: I can't seem to see any EGL extensions which allow you to
connect a Wayland surface as an EGLStream consumer. Do you have
something else unpublished that's being used here, or is this what the
libnvidia-egl-wayland library is for? Or do you just have clients
using EGLSurfaces as normal, which happen to be implemented internally
as EGLStreams? (Also, that the only way to test this is through
proprietary drivers implementing only-just-published extensions not
only makes me very sad, but hugely increases the potential for this to
be inadvertently broken.)

> Thus, a compositor could produce frames and feed them to an
> EGLOutputLayer through an EGLStream for presentation on a display
> device.
>
> In a similar way, by attaching a GLTexture consumer to a stream, a
> producer (wayland client) could feed frames to a texture, which in
> turn can be used by a compositor to prepare the final frame to be
> presented.

Quick aside: this reminds me in many unfortunate ways of

Re: Introduction and updates from NVIDIA

[Nicole Fontenot]

Hello Miguel,

I cannot comment on if these patches are within scope of wayland but, I
think now is the perfect time to consider API extensions.

It would be great if someone with an Nvidia card has time to run
performance tests when you submit your patches. A proper decision to
include the patches has more likelihood of happening that way, I think.
If they do not get accepted into Wayland  I'm sure that users of Nvidia
cards, particularly the steam users, would still want this as an extension
in their favorite composer.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3] input: use doubles in the interfaces to notify of input events

[Giulio Camuffo]

2016-03-22 14:32 GMT+02:00 Pekka Paalanen :
> On Mon, 21 Mar 2016 13:29:00 +0200
> Giulio Camuffo  wrote:
>
>> This patch is a further step in the wl_fixed_t internal sanitization.
>> It changes the notify_* functions to take doubles instead of wl_fixed_t
>> but does not change how these are stored in the various input structs
>> yet, except for weston_pointer_axis_event.
>> However this already allows to remove all wl_fixed_t usage in places
>> like the libinput or the x11 backend.
>> ---
>>
>> v3: rebased on master. This required quite many changes, due to the
>> introduction of the input events
>>
>>  desktop-shell/shell.c|  2 +-
>>  src/compositor-rdp.c | 12 +++---
>>  src/compositor-wayland.c | 58 
>> ++--
>>  src/compositor-x11.c | 22 +-
>>  src/compositor.c | 12 +-
>>  src/compositor.h | 12 +-
>>  src/input.c  | 15 -
>>  src/libinput-device.c| 26 +-
>>  src/screen-share.c   |  5 +++--
>>  9 files changed, 86 insertions(+), 78 deletions(-)
>>
>
> Hi Giulio,
>
> I see you already sent v4, so I'll leave reviewing v3 unfinished. Here
> are my comments so far.
>
> I have checked these changes:
> - weston_pointer_axis_event::value
> - notify_motion_absolute()
>
>> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
>> index e1fefae..837c18b 100644
>> --- a/desktop-shell/shell.c
>> +++ b/desktop-shell/shell.c
>> @@ -4786,7 +4786,7 @@ surface_opacity_binding(struct weston_pointer 
>> *pointer, uint32_t time,
>>   if (!shsurf)
>>   return;
>>
>> - shsurf->view->alpha -= wl_fixed_to_double(event->value) * step;
>> + shsurf->view->alpha -= event->value * step;
>>
>>   if (shsurf->view->alpha > 1.0)
>>   shsurf->view->alpha = 1.0;
>
> Desktop-shell is missing another place: do_zoom() expects a wl_fixed_t,
> but gets fed a event->value directly.

Ah, good finding.

>
> I found it my making weston_pointer_axis_event::value
> __attribute__((deprecated)) and inspecting all the sites it raises.
>
>
>
>> diff --git a/src/input.c b/src/input.c
>> index 5d13b08..f52db4b 100644
>> --- a/src/input.c
>> +++ b/src/input.c
>> @@ -352,7 +352,8 @@ weston_pointer_send_axis(struct weston_pointer *pointer,
>>
>>   if (event->value)
>>   wl_pointer_send_axis(resource, time,
>> -  event->axis, event->value);
>> +  event->axis,
>> +  
>> wl_fixed_from_double(event->value));
>
> The change is fine, but I just wanted to point out the 'if
> (event->value)' test. I suppose it's fine even when it's a double,
> right? We don't need a threshold check?

Yeah, i think it's fine. Even if it's a 1e-10 i don't think there's
harm in sending it.


Thanks,
Giulio

>
>>   else if (wl_resource_get_version(resource) >=
>>WL_POINTER_AXIS_STOP_SINCE_VERSION)
>>   wl_pointer_send_axis_stop(resource, time,
>
>
> Thanks,
> pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3] input: use doubles in the interfaces to notify of input events

[Pekka Paalanen]

On Mon, 21 Mar 2016 13:29:00 +0200
Giulio Camuffo  wrote:

> This patch is a further step in the wl_fixed_t internal sanitization.
> It changes the notify_* functions to take doubles instead of wl_fixed_t
> but does not change how these are stored in the various input structs
> yet, except for weston_pointer_axis_event.
> However this already allows to remove all wl_fixed_t usage in places
> like the libinput or the x11 backend.
> ---
> 
> v3: rebased on master. This required quite many changes, due to the
> introduction of the input events
> 
>  desktop-shell/shell.c|  2 +-
>  src/compositor-rdp.c | 12 +++---
>  src/compositor-wayland.c | 58 
> ++--
>  src/compositor-x11.c | 22 +-
>  src/compositor.c | 12 +-
>  src/compositor.h | 12 +-
>  src/input.c  | 15 -
>  src/libinput-device.c| 26 +-
>  src/screen-share.c   |  5 +++--
>  9 files changed, 86 insertions(+), 78 deletions(-)
> 

Hi Giulio,

I see you already sent v4, so I'll leave reviewing v3 unfinished. Here
are my comments so far.

I have checked these changes:
- weston_pointer_axis_event::value
- notify_motion_absolute()

> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index e1fefae..837c18b 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -4786,7 +4786,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
> uint32_t time,
>   if (!shsurf)
>   return;
>  
> - shsurf->view->alpha -= wl_fixed_to_double(event->value) * step;
> + shsurf->view->alpha -= event->value * step;
>  
>   if (shsurf->view->alpha > 1.0)
>   shsurf->view->alpha = 1.0;

Desktop-shell is missing another place: do_zoom() expects a wl_fixed_t,
but gets fed a event->value directly.

I found it my making weston_pointer_axis_event::value
__attribute__((deprecated)) and inspecting all the sites it raises.



> diff --git a/src/input.c b/src/input.c
> index 5d13b08..f52db4b 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -352,7 +352,8 @@ weston_pointer_send_axis(struct weston_pointer *pointer,
>  
>   if (event->value)
>   wl_pointer_send_axis(resource, time,
> -  event->axis, event->value);
> +  event->axis,
> +  
> wl_fixed_from_double(event->value));

The change is fine, but I just wanted to point out the 'if
(event->value)' test. I suppose it's fine even when it's a double,
right? We don't need a threshold check?

>   else if (wl_resource_get_version(resource) >=
>WL_POINTER_AXIS_STOP_SINCE_VERSION)
>   wl_pointer_send_axis_stop(resource, time,


Thanks,
pq


pgpViJXsOR1uk.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 6/7] compositor-drm: Renaming of things

[Pekka Paalanen]

On Tue, 22 Mar 2016 08:49:11 +
Daniel Stone  wrote:

> Hi,
> 
> On 21 March 2016 at 16:41, Miguel A. Vico  wrote:
> > In preparation for follow-on changes to support frame presentation
> > through EGLDevice+EGLOutput, this change includes the following:
> >   - Rename drm_backend::format   to gbm_format
> >   - Rename drm_output::formatto gbm_format
> >   - Rename drm_output::surface   to gbm_surface
> >   - Rename drm_output::cursor_bo to gbm_cursor_bo  
> 
> I think this is actually an improvement even without Streams.
> 
> Reviewed-by: Daniel Stone 

Hi,

yeah, this patch rebased out of the series quite trivially too, so
pushed:
cc3a192..fcf4b6c  master -> master

I took the liberty of making the subject a little more explicit.


Thanks,
pq


pgpCftpdKMIIf.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/7] gl-renderer: Implement & use check_extension

[Pekka Paalanen]

On Tue, 22 Mar 2016 08:42:41 +
Daniel Stone  wrote:

> Hi Miguel,
> 
> On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> > Using strstr(3) for checking for extensions is an error-prone mechanism
> > as extension names can be prefixes of other extension names (see
> > https://www.opengl.org/registry/doc/rules.html#using).
> >
> > This change implements the check_extension() function to properly check
> > for an extension and replaces all usages of strstr(3).  
> 
> Looks good to me, thanks for this!
> 
> Reviewed-by: Daniel Stone 

Hi,

as this is a nice fix and applies on its own, pushed:
   c6459c49..cc3a192 master -> master


Thanks,
pq


pgpBufw6sC7A3.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 7/7] compositor-drm: Add support for EGLDevice+EGLOutput

[Daniel Stone]

Hi,

On 21 March 2016 at 16:41, Miguel A. Vico  wrote:
> As previously stated, EGLDevice and EGLOutput will provide means
> to access native device objects and different portions of display
> control hardware respectively.
>
> Whenever EGL_EXT_device_drm extension is present, EGLDevice can
> be used to enumerate and access DRM KMS devices, and EGLOutputLayer
> to enumerate and access DRM KMS crtcs and planes.

EGLDevice isn't enumerating devices though: you still use udev to
enumerate DRM devices, and then walk the list of EGLDevices to find
the device you've already settled on. EGL_DRM_MASTER_FD_EXT then gets
used to inject an open connection back in to replace the device you've
already opened. Messy. :\

> By using EGLStreams and attaching an EGLOutputLayer consumer
> (representing a DRM KMS crtc or plane) to it, compositor-drm can
> produce final composition frames and present them on a DRM device.

It's gl-renderer which produces the frames, really.

> This change adds required logic to support presentation through
> EGLDevice+EGLOutput+EGLStream. Whether GBM or EGLDevice should be
> used can be controlled by --use-egldevice backend argument.

Should this not be automatic, i.e. use gbm if platform_gbm is present
and EGLDevice if gbm isn't present but the other extensions are?

> @@ -531,17 +540,21 @@ drm_output_render_gl(struct drm_output *output, 
> pixman_region32_t *damage)
> output->base.compositor->renderer->repaint_output(>base,
>   damage);
>
> -   bo = gbm_surface_lock_front_buffer(output->gbm_surface);
> -   if (!bo) {
> -   weston_log("failed to lock front buffer: %m\n");
> -   return;
> -   }
> +   if (b->use_egldevice)
> +   output->next = output->dumb[0];

Huh?

I'm assuming you use two (never actually used) dumb BOs as
output->{current,next} to fit in with compositor-drm's bookkeeping,
whilst Streams handles the actual buffer management for you, right?
That seems like quite a hack.

>  /* When initializing EGL, if the preferred buffer format isn't available
>   * we may be able to susbstitute an ARGB format for an XRGB one.

This seems like it doesn't apply
 changed the logic to al
> @@ -1594,38 +1642,59 @@ fallback_format_for(uint32_t format)
>  static int
>  drm_backend_create_gl_renderer(struct drm_backend *b)
>  {
> -   EGLint format[3] = {
> +   EGLint platform_attribs[] = {
> +   EGL_DRM_MASTER_FD_EXT, b->drm.fd,
> +   EGL_NONE
> +   };

Seems like this should be called device_platform_attribs, since
they're only used in the EGLDevice case, not gbm.

> +   EGLint format[] = {
> b->gbm_format,
> fallback_format_for(b->gbm_format),
> -   0,
> +   0

Unrelated changes to the format array here.

> +   if (b->use_egldevice) {
> +   b->egldevice = create_egldevice(b->drm.filename);
> +   if (b->egldevice == EGL_NO_DEVICE_EXT)
> +   return -1;
> +   } else {
> +   b->gbm = create_gbm_device(b->drm.fd);
> +   if (!b->gbm)
> +   return -1;
> +   }
> +
> if (drm_backend_create_gl_renderer(b) < 0) {
> -   gbm_device_destroy(b->gbm);
> +   if (b->gbm)
> +   gbm_device_destroy(b->gbm);

Shouldn't the egldevice be destroyed here too?

> +   if (b->use_egldevice) {
> +   int w = output->base.current_mode->width;
> +   int h = output->base.current_mode->height;
>
> -   if (format[1])
> -   n_formats = 2;
> -   if (gl_renderer->output_window_create(>base,
> -  
> (EGLNativeWindowType)output->gbm_surface,
> -  output->gbm_surface,
> -  gl_renderer->opaque_attribs,
> -  format,
> -  n_formats) < 0) {
> -   weston_log("failed to create gl renderer output state\n");
> -   gbm_surface_destroy(output->gbm_surface);
> -   return -1;
> -   }
> +   /* Create a dumb fb for modesetting */
> +   output->dumb[0] = drm_fb_create_dumb(b, w, h);
> +   if (!output->dumb[0]) {
> +   weston_log("failed to create dumb framebuffer\n");
> +   return -1;
> +   }

Seems I was correct earlier - if it's being passed to drmModeSetCrtc
though, this will mean that we flash up some completely unknown
content when starting. Not pretty.

> +   } else {
> +   EGLint format[2] = {
> +   output->gbm_format,
> +   fallback_format_for(output->gbm_format),
> +   };
> +   int i, flags, n_formats = 1;
> +
> +   

[PATCH weston v4] input: use doubles in the interfaces to notify of input events

[Giulio Camuffo]

This patch is a further step in the wl_fixed_t internal sanitization.
It changes the notify_* functions to take doubles instead of wl_fixed_t
but does not change how these are stored in the various input structs
yet, except for weston_pointer_axis_event.
However this already allows to remove all wl_fixed_t usage in places
like the libinput or the x11 backend.

Reviewed-by: Daniel Stone 
---

v4: - removed redundant conversion fixed->double
- fixed newline alignment in input_handle_touch_motion definition

 desktop-shell/shell.c|  2 +-
 src/compositor-rdp.c | 12 +++
 src/compositor-wayland.c | 56 +---
 src/compositor-x11.c | 22 +--
 src/compositor.c | 12 +--
 src/compositor.h | 12 +--
 src/input.c  | 15 +++--
 src/libinput-device.c| 26 ++
 src/screen-share.c   |  5 +++--
 9 files changed, 84 insertions(+), 78 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e1fefae..837c18b 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4786,7 +4786,7 @@ surface_opacity_binding(struct weston_pointer *pointer, 
uint32_t time,
if (!shsurf)
return;
 
-   shsurf->view->alpha -= wl_fixed_to_double(event->value) * step;
+   shsurf->view->alpha -= event->value * step;
 
if (shsurf->view->alpha > 1.0)
shsurf->view->alpha = 1.0;
diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
index e603b76..773b6b5 100644
--- a/src/compositor-rdp.c
+++ b/src/compositor-rdp.c
@@ -942,7 +942,6 @@ static BOOL xf_peer_post_connect(freerdp_peer *client)
 static FREERDP_CB_RET_TYPE
 xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
uint32_t button = 0;
@@ -951,10 +950,8 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
if (flags & PTR_FLAGS_MOVE) {
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
need_frame = true;
}
}
@@ -988,7 +985,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
value = -value;
 
weston_event.axis = WL_POINTER_AXIS_VERTICAL_SCROLL;
-   weston_event.value = 
wl_fixed_from_double(DEFAULT_AXIS_STEP_DISTANCE * value);
+   weston_event.value = DEFAULT_AXIS_STEP_DISTANCE * value;
weston_event.discrete = (int)value;
weston_event.has_discrete = true;
 
@@ -1006,16 +1003,13 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
UINT16 y)
 static FREERDP_CB_RET_TYPE
 xf_extendedMouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
 {
-   wl_fixed_t wl_x, wl_y;
RdpPeerContext *peerContext = (RdpPeerContext *)input->context;
struct rdp_output *output;
 
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
-   wl_x = wl_fixed_from_int((int)x);
-   wl_y = wl_fixed_from_int((int)y);
notify_motion_absolute(>item.seat, 
weston_compositor_get_time(),
-   wl_x, wl_y);
+   x, y);
}
 
FREERDP_CB_RETURN(TRUE);
diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
index 8a1878c..9d1a251 100644
--- a/src/compositor-wayland.c
+++ b/src/compositor-wayland.c
@@ -1275,11 +1275,15 @@ input_set_cursor(struct wayland_input *input)
 static void
 input_handle_pointer_enter(void *data, struct wl_pointer *pointer,
   uint32_t serial, struct wl_surface *surface,
-  wl_fixed_t x, wl_fixed_t y)
+  wl_fixed_t fixed_x, wl_fixed_t fixed_y)
 {
struct wayland_input *input = data;
int32_t fx, fy;
enum theme_location location;
+   double x, y;
+
+   x = wl_fixed_to_double(fixed_x);
+   y = wl_fixed_to_double(fixed_y);
 
/* XXX: If we get a modifier event immediately before the focus,
 *  we should try to keep the same serial. */
@@ -1288,11 +1292,10 @@ input_handle_pointer_enter(void *data, struct 
wl_pointer *pointer,
 
if (input->output->frame) {
location = frame_pointer_enter(input->output->frame, input,
-  wl_fixed_to_int(x),
- 

Re: [PATCH weston v3] input: use doubles in the interfaces to notify of input events

[Giulio Camuffo]

2016-03-22 11:47 GMT+02:00 Daniel Stone :
> Hey Giulio,
>
> On 21 March 2016 at 11:29, Giulio Camuffo  wrote:
>> @@ -1275,11 +1275,15 @@ input_set_cursor(struct wayland_input *input)
>>  static void
>>  input_handle_pointer_enter(void *data, struct wl_pointer *pointer,
>>uint32_t serial, struct wl_surface *surface,
>> -  wl_fixed_t x, wl_fixed_t y)
>> +  wl_fixed_t fixed_x, wl_fixed_t fixed_y)
>>  {
>> struct wayland_input *input = data;
>> int32_t fx, fy;
>> enum theme_location location;
>> +   double x, y;
>> +
>> +   x = wl_fixed_to_double(fixed_x);
>> +   y = wl_fixed_to_double(fixed_y);
>>
>> /* XXX: If we get a modifier event immediately before the focus,
>>  *  we should try to keep the same serial. */
>> @@ -1288,11 +1292,10 @@ input_handle_pointer_enter(void *data, struct 
>> wl_pointer *pointer,
>>
>> if (input->output->frame) {
>> location = frame_pointer_enter(input->output->frame, input,
>> -  wl_fixed_to_int(x),
>> -  wl_fixed_to_int(y));
>> +  x, y);
>> frame_interior(input->output->frame, , , NULL, NULL);
>> -   x -= wl_fixed_from_int(fx);
>> -   y -= wl_fixed_from_int(fy);
>> +   x -= fx;
>> +   y -= fy;
>>
>> if (frame_status(input->output->frame) & 
>> FRAME_STATUS_REPAINT)
>> weston_output_schedule_repaint(>output->base);
>> @@ -1300,6 +1303,8 @@ input_handle_pointer_enter(void *data, struct 
>> wl_pointer *pointer,
>> location = THEME_LOCATION_CLIENT_AREA;
>> }
>>
>> +   x = wl_fixed_to_double(fixed_x);
>> +   y = wl_fixed_to_double(fixed_y);
>> weston_output_transform_coordinate(>output->base, x, y, , 
>> );
>
> I think the second pair of wl_fixed_to_double calls here is broken; at
> the very least, it's a change in behaviour from previously.

Ah, you're right. It's redundant at least.

Cheers,
Giulio

>
> The rest, as far as I can tell (had to dig a little bit to convince
> myself that the grab interface didn't require any changes), looks
> good:
> Reviewed-by: Daniel Stone 
>
> Thanks for taking this on!
>
> Cheers,
> Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 4/7] gl-renderer: Add support for EGLDevice+EGLOutput

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> EGLDevice provides means to enumerate native devices, and then create
> an EGL display connection from them.
>
> Similarly, EGLOutput will provide means to access different
> portions of display control hardware associated with an EGLDevice.
>
> For instance, EGLOutputLayer represents a portion of display
> control hardware that accepts an image as input and processes it
> for presentation on a display device.
>
> EGLStream implements a mechanism to communicate frame producers and
> frame consumers. By attaching an EGLOutputLayer consumer to a stream,
> a producer will be able to present frames on a display device.
>
> Thus, a compositor could produce frames and feed them to an
> EGLOutputLayer through an EGLStream for presentation on a display
> device.
>
> In a similar way, by attaching a GLTexture consumer to a stream, a
> producer (wayland client) could feed frames to a texture, which in
> turn can be used by a compositor to prepare the final frame to be
> presented.
>
> This change adds required logic to support presentation approach
> described above.
>
> Note that some unpublished EGL extensions were needed:
>
>  - EGL_NV_stream_attrib:
>
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_stream_attrib.txt
>
>  - EGL_EXT_stream_acquire_mode:
>
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_EXT_stream_acquire_mode.txt
>
>  - EGL_NV_output_drm_flip_event:
>
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_output_drm_flip_event.txt
>
> Also, in order to allow wl_buffers to be bound to EGLStreams, we
> kludged eglQueryWaylandBufferWL(EGL_WAYLAND_BUFFER_WL) to return
> the stream file descriptor.

Ugh, that's pretty unpleasant.

> We think the proper way to handle this should be:
>
>  - Update WL_bind_wayland_display such that eglQueryWaylandBufferWL() accepts
>a new attribute EGL_WAYLAND_BUFFER_TYPE_WL, returning
>EGL_WAYLAND_BUFFER_EGLIMAGE_WL for the non-stream case.
>
>  - Add a new WL_wayland_buffer_eglstream extension, which would define
>EGL_WAYLAND_BUFFER_EGLSTREAM_WL as a return value for
>EGL_WAYLAND_BUFFER_TYPE_WL, and yet another attribute
>EGL_WAYLAND_BUFFER_EGLSTREAM_FD_WL to query the stream file descriptor.

I disagree quite strongly with this, since it has unpleasant
compatibility implications. I'd rather see a new EGL_WAYLAND_STREAM_WL
token or similar, since a stream is by definition not a buffer.

> @@ -84,6 +85,8 @@ struct gl_output_state {
> struct gl_border_image borders[4];
> enum gl_border_status border_status;
>
> +   EGLStreamKHR egl_stream;
> +
> struct weston_matrix output_matrix;
>  };
>
> @@ -159,6 +162,9 @@ struct gl_surface_state {
> int height; /* in pixels */
> int y_inverted;
>
> +   EGLStreamKHR egl_stream;
> +   bool stream_frame_acquired;
> +
> struct weston_surface *surface;
>
> struct wl_listener surface_destroy_listener;
> @@ -202,6 +208,36 @@ struct gl_renderer {
>
> int has_configless_context;
>
> +   PFNEGLCREATESTREAMKHRPROC create_stream;
> +   PFNEGLDESTROYSTREAMKHRPROC destroy_stream;
> +   PFNEGLQUERYSTREAMKHRPROC query_stream;
> +   int has_egl_stream;
> +
> +   PFNEGLCREATESTREAMPRODUCERSURFACEKHRPROC 
> create_stream_producer_surface;
> +   int has_egl_stream_producer_eglsurface;
> +
> +   PFNEGLSTREAMCONSUMEROUTPUTEXTPROC stream_consumer_output;
> +   int has_egl_stream_consumer_egloutput;
> +
> +   PFNEGLSTREAMCONSUMERACQUIREKHRPROC stream_consumer_acquire;
> +#ifdef EGL_EXT_stream_acquire_mode
> +   PFNEGLSTREAMCONSUMERACQUIREATTRIBEXTPROC 
> stream_consumer_acquire_attrib;
> +#endif
> +   int has_egl_stream_acquire_mode;
> +
> +   int has_egl_output_drm;
> +   int has_egl_output_drm_flip_event;
> +
> +   PFNEGLGETOUTPUTLAYERSEXTPROC get_output_layers;
> +   PFNEGLQUERYOUTPUTLAYERATTRIBEXTPROC query_output_layer_attrib;
> +   int has_egl_output_base;
> +
> +   PFNEGLCREATESTREAMFROMFILEDESCRIPTORKHRPROC 
> create_stream_from_file_descriptor;
> +   int has_egl_stream_cross_process_fd;
> +
> +   PFNEGLSTREAMCONSUMERGLTEXTUREEXTERNALKHRPROC 
> stream_consumer_gltexture;
> +   int has_egl_stream_consumer_gltexture;
> +

These all need conditional declarations in headers, to support
building with other EGL implementations.

> @@ -743,6 +780,21 @@ draw_view(struct weston_view *ev, struct weston_output 
> *output,
> if (!gs->shader)
> return;
>
> +   /* If using EGLStreams, only continue if we are certain we have 
> something to
> +* render, i.e. a new frame is available or an old one was previously
> +* acquired and is ready to be re-used */
> +   if (gs->egl_stream != EGL_NO_STREAM_KHR) {
> +   if 

Re: [PATCH weston v3] input: use doubles in the interfaces to notify of input events

[Daniel Stone]

Hey Giulio,

On 21 March 2016 at 11:29, Giulio Camuffo  wrote:
> @@ -1275,11 +1275,15 @@ input_set_cursor(struct wayland_input *input)
>  static void
>  input_handle_pointer_enter(void *data, struct wl_pointer *pointer,
>uint32_t serial, struct wl_surface *surface,
> -  wl_fixed_t x, wl_fixed_t y)
> +  wl_fixed_t fixed_x, wl_fixed_t fixed_y)
>  {
> struct wayland_input *input = data;
> int32_t fx, fy;
> enum theme_location location;
> +   double x, y;
> +
> +   x = wl_fixed_to_double(fixed_x);
> +   y = wl_fixed_to_double(fixed_y);
>
> /* XXX: If we get a modifier event immediately before the focus,
>  *  we should try to keep the same serial. */
> @@ -1288,11 +1292,10 @@ input_handle_pointer_enter(void *data, struct 
> wl_pointer *pointer,
>
> if (input->output->frame) {
> location = frame_pointer_enter(input->output->frame, input,
> -  wl_fixed_to_int(x),
> -  wl_fixed_to_int(y));
> +  x, y);
> frame_interior(input->output->frame, , , NULL, NULL);
> -   x -= wl_fixed_from_int(fx);
> -   y -= wl_fixed_from_int(fy);
> +   x -= fx;
> +   y -= fy;
>
> if (frame_status(input->output->frame) & FRAME_STATUS_REPAINT)
> weston_output_schedule_repaint(>output->base);
> @@ -1300,6 +1303,8 @@ input_handle_pointer_enter(void *data, struct 
> wl_pointer *pointer,
> location = THEME_LOCATION_CLIENT_AREA;
> }
>
> +   x = wl_fixed_to_double(fixed_x);
> +   y = wl_fixed_to_double(fixed_y);
> weston_output_transform_coordinate(>output->base, x, y, , 
> );

I think the second pair of wl_fixed_to_double calls here is broken; at
the very least, it's a change in behaviour from previously.

The rest, as far as I can tell (had to dig a little bit to convince
myself that the grab interface didn't require any changes), looks
good:
Reviewed-by: Daniel Stone 

Thanks for taking this on!

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 5/7] compositor-drm: Gracefully handle vblank and flip invalid timestamps

[Daniel Stone]

Hi,

On 22 March 2016 at 08:50, Daniel Vetter  wrote:
> On Tue, Mar 22, 2016 at 08:47:12AM +, Daniel Stone wrote:
>> This is odd. Instant queries failing is fine, but in which
>> circumstances can you fail to return a valid timestamp in a pageflip
>> event ... ? I don't know of any other kernel driver which does this -
>> at the very least, if you don't have a hardware timestamp, then you
>> log the time in your kernel driver which handles the hardware event
>> (e.g. softirq handler), and use that instead.
>
> nouveau bugs iirc. Yes, should never happen, it's a bug in the kernel
> driver that iirc Mario recently tracked down. nouveau somehow started the
> vblank machinery before the pipe was up, or something like that.
>
> So yeah happens, I'm not sure we want to let kernel drivers get away with
> plain broken behavior forever. I'm leaning towards nack, pls fix the
> kernel driver.

Yes, particularly as no-one has reported it on any other drivers. I
don't think it's behaviour we really want to support.

> And maybe make sure the corresponding igt pass ;-)

For reference, igt here is intel-gpu-tools, which despite its name is
actually a generic KMS test/conformance suite. Most of the tests are
still Intel-specific, but we are actively working on making them
generic - I was actually in the middle of reviewing a patchset to make
a rather larger chunk of it generic when I got distracted by this
thread. ;) It's a great way to find out ways in which your driver
breaks userspace expectations of a 'well-behaved' KMS driver early.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 1/7] gl-renderer: Renaming of things and minor improvements

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> In preparation for follow-on changes to support frame presentation
> through EGLDevice+EGLOutput, this change includes the following:
>   -Rename gl_renderer_output_create to gl_renderer_output_window_create
>   -Add  argument to gl_renderer_create
>   -Rename  argument for gl_renderer_create() and
>gl_renderer_output_window_create() to 
>   -Accept non-NULL empty  arrays (n_ids == 0) both in
>gl_renderer_create() and gl_renderer_output_window_create()

It would be nice to split these up into discrete changes: one per
bullet point is a pretty good guideline. While you're at it,
gl_renderer->create() could definitely be renamed to something more
usefully specific, e.g. gl_render->display_create.

> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index cb08344..0c55e0b 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -2493,7 +2493,7 @@ egl_choose_config(struct gl_renderer *gr, const EGLint 
> *attribs,
> goto out;
> }
>
> -   if (!visual_id)
> +   if (!visual_id || n_ids == 0)
> config_index = 0;
>
> for (i = 0; config_index == -1 && i < n_ids; i++)

This bit I'm a bit confused about though. How do you end up in this situation?

The rest look good to me, once split into separate patches.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 2/7] gl-renderer: Refactor gl_renderer_output_window_create()

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> +   egl_surface = gl_renderer_create_window_surface(gr,
> +   window_for_legacy,
> +   window_for_platform,
> +   config_attribs,
> +   visual_id, n_ids);
> +
> +   ret = gl_renderer_output_create(output, egl_surface);
> +   if (ret < 0 && egl_surface != EGL_NO_SURFACE)
> +   eglDestroySurface(gr->egl_display, egl_surface);

I'm not a huge fan of the error-handling here: I'd rather see an
instant return -1 from this function if we can't create an EGLSurface,
rather than relying on gl_renderer_output_create to check and handle
it for us.

Otherwise I guess this looks OK.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 5/7] compositor-drm: Gracefully handle vblank and flip invalid timestamps

[Daniel Vetter]

On Tue, Mar 22, 2016 at 08:47:12AM +, Daniel Stone wrote:
> Hi Miguel,
> 
> On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> > Instant query for vblank timestamp may always fail, resulting in
> > never scheduling a full repaint in drm_output_start_repaint_loop().
> >
> > Additionally, timestamp provided in page_flip_handler() may also be
> > invalid.
> >
> > [...]
> >
> > @@ -869,6 +878,15 @@ page_flip_handler(int fd, unsigned int frame,
> > else if (!output->vblank_pending) {
> > ts.tv_sec = sec;
> > ts.tv_nsec = usec * 1000;
> > +
> > +   /* Zero timestamp means failure to get valid timestamp, so 
> > immediately
> > +* finish frame */
> > +   if (ts.tv_sec == 0 && ts.tv_nsec == 0) {
> > +   
> > weston_compositor_read_presentation_clock(output->base.compositor,
> > + );
> > +   flags = WP_PRESENTATION_FEEDBACK_INVALID;
> > +   }
> > +
> > weston_output_finish_frame(>base, , flags);
> >
> > /* We can't call this from frame_notify, because the 
> > output's
> 
> This is odd. Instant queries failing is fine, but in which
> circumstances can you fail to return a valid timestamp in a pageflip
> event ... ? I don't know of any other kernel driver which does this -
> at the very least, if you don't have a hardware timestamp, then you
> log the time in your kernel driver which handles the hardware event
> (e.g. softirq handler), and use that instead.

nouveau bugs iirc. Yes, should never happen, it's a bug in the kernel
driver that iirc Mario recently tracked down. nouveau somehow started the
vblank machinery before the pipe was up, or something like that.

So yeah happens, I'm not sure we want to let kernel drivers get away with
plain broken behavior forever. I'm leaning towards nack, pls fix the
kernel driver. And maybe make sure the corresponding igt pass ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 6/7] compositor-drm: Renaming of things

[Daniel Stone]

Hi,

On 21 March 2016 at 16:41, Miguel A. Vico  wrote:
> In preparation for follow-on changes to support frame presentation
> through EGLDevice+EGLOutput, this change includes the following:
>   - Rename drm_backend::format   to gbm_format
>   - Rename drm_output::formatto gbm_format
>   - Rename drm_output::surface   to gbm_surface
>   - Rename drm_output::cursor_bo to gbm_cursor_bo

I think this is actually an improvement even without Streams.

Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 5/7] compositor-drm: Gracefully handle vblank and flip invalid timestamps

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> Instant query for vblank timestamp may always fail, resulting in
> never scheduling a full repaint in drm_output_start_repaint_loop().
>
> Additionally, timestamp provided in page_flip_handler() may also be
> invalid.
>
> [...]
>
> @@ -869,6 +878,15 @@ page_flip_handler(int fd, unsigned int frame,
> else if (!output->vblank_pending) {
> ts.tv_sec = sec;
> ts.tv_nsec = usec * 1000;
> +
> +   /* Zero timestamp means failure to get valid timestamp, so 
> immediately
> +* finish frame */
> +   if (ts.tv_sec == 0 && ts.tv_nsec == 0) {
> +   
> weston_compositor_read_presentation_clock(output->base.compositor,
> + );
> +   flags = WP_PRESENTATION_FEEDBACK_INVALID;
> +   }
> +
> weston_output_finish_frame(>base, , flags);
>
> /* We can't call this from frame_notify, because the output's

This is odd. Instant queries failing is fine, but in which
circumstances can you fail to return a valid timestamp in a pageflip
event ... ? I don't know of any other kernel driver which does this -
at the very least, if you don't have a hardware timestamp, then you
log the time in your kernel driver which handles the hardware event
(e.g. softirq handler), and use that instead.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/7] gl-renderer: Implement & use check_extension

[Daniel Stone]

Hi Miguel,

On 21 March 2016 at 16:37, Miguel A. Vico  wrote:
> Using strstr(3) for checking for extensions is an error-prone mechanism
> as extension names can be prefixes of other extension names (see
> https://www.opengl.org/registry/doc/rules.html#using).
>
> This change implements the check_extension() function to properly check
> for an extension and replaces all usages of strstr(3).

Looks good to me, thanks for this!

Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 7/7] compositor-drm: Add support for EGLDevice+EGLOutput

[Daniel Vetter]

On Mon, Mar 21, 2016 at 05:41:32PM +0100, Miguel A. Vico wrote:
> As previously stated, EGLDevice and EGLOutput will provide means
> to access native device objects and different portions of display
> control hardware respectively.
> 
> Whenever EGL_EXT_device_drm extension is present, EGLDevice can
> be used to enumerate and access DRM KMS devices, and EGLOutputLayer
> to enumerate and access DRM KMS crtcs and planes.
> 
> By using EGLStreams and attaching an EGLOutputLayer consumer
> (representing a DRM KMS crtc or plane) to it, compositor-drm can
> produce final composition frames and present them on a DRM device.
> 
> This change adds required logic to support presentation through
> EGLDevice+EGLOutput+EGLStream. Whether GBM or EGLDevice should be
> used can be controlled by --use-egldevice backend argument.
> 
> Signed-off-by: Miguel A Vico Moya 
> Reviewed-by: Andy Ritger 
> Reviewed-by: Adam Cheney 

Andy did a presentation about these extensions at xdc 2014, and consensus
reply from folks (not just me) was "no way". I've seen a few other
discussions fly by meanwhile, with exactly the same opinions.

If you really can't have a display driver in the kernel, why not fake
proper drm with something like CUSE (or well similar, probably need to
interface with at least dma-bufs from the kernel blob somwehere)? Or just
like use the existing kernel driver and improve that to suit your needs,
like everyone else at least plans to.

I don't expect you'll ever be able to sell this to the community and get
it merged.
-Daniel

> ---
>  src/compositor-drm.c | 319 
> +++
>  src/main.c   |   1 +
>  2 files changed, 224 insertions(+), 96 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index ecb340d..82097ff 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -75,6 +75,10 @@
>  #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
>  #endif
>  
> +#ifndef EGL_DRM_MASTER_FD_EXT
> +#define EGL_DRM_MASTER_FD_EXT 0x333C
> +#endif
> +
>  static int option_current_mode = 0;
>  
>  enum output_config {
> @@ -101,7 +105,10 @@ struct drm_backend {
>   int fd;
>   char *filename;
>   } drm;
> +
> + EGLDeviceEXT egldevice;
>   struct gbm_device *gbm;
> +
>   uint32_t *crtcs;
>   int num_crtcs;
>   uint32_t crtc_allocator;
> @@ -124,6 +131,7 @@ struct drm_backend {
>   int cursors_are_broken;
>  
>   int use_pixman;
> + int use_egldevice;
>  
>   uint32_t prev_state;
>  
> @@ -225,6 +233,7 @@ struct drm_parameters {
>   int connector;
>   int tty;
>   int use_pixman;
> + int use_egldevice;
>   const char *seat_id;
>  };
>  
> @@ -531,17 +540,21 @@ drm_output_render_gl(struct drm_output *output, 
> pixman_region32_t *damage)
>   output->base.compositor->renderer->repaint_output(>base,
> damage);
>  
> - bo = gbm_surface_lock_front_buffer(output->gbm_surface);
> - if (!bo) {
> - weston_log("failed to lock front buffer: %m\n");
> - return;
> - }
> + if (b->use_egldevice)
> + output->next = output->dumb[0];
> + else {
> + bo = gbm_surface_lock_front_buffer(output->gbm_surface);
> + if (!bo) {
> + weston_log("failed to lock front buffer: %m\n");
> + return;
> + }
>  
> - output->next = drm_fb_get_from_bo(bo, b, output->gbm_format);
> - if (!output->next) {
> - weston_log("failed to get drm_fb for bo\n");
> - gbm_surface_release_buffer(output->gbm_surface, bo);
> - return;
> + output->next = drm_fb_get_from_bo(bo, b, output->gbm_format);
> + if (!output->next) {
> + weston_log("failed to get drm_fb for bo\n");
> + gbm_surface_release_buffer(output->gbm_surface, bo);
> + return;
> + }
>   }
>  }
>  
> @@ -666,9 +679,14 @@ drm_output_repaint(struct weston_output *output_base,
>   output_base->set_dpms(output_base, WESTON_DPMS_ON);
>   }
>  
> - if (drmModePageFlip(backend->drm.fd, output->crtc_id,
> - output->next->fb_id,
> - DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> + if (backend->use_egldevice)
> + ret = gl_renderer->output_stream_flip(>base, output);
> + else
> + ret = drmModePageFlip(backend->drm.fd, output->crtc_id,
> +   output->next->fb_id,
> +   DRM_MODE_PAGE_FLIP_EVENT, output);
> +
> + if (ret < 0) {
>   weston_log("queueing pageflip failed: %m\n");
>   goto err_pageflip;
>   }
> @@ -739,7 +757,6 @@ drm_output_start_repaint_loop(struct weston_output 
>