Re: [RFC xserver v4 04/14] present: Send PresentCompleteModeSuboptimalCopy appropriately

[Michel Dänzer]

On 16/10/17 07:02 AM, Louis-Francis Ratté-Boulianne wrote:
> Add 'check_flip2' hook for driver to let know the core
> about why flipping is not possible ('reason').
> If it is because of unsupported buffer format/modifier,
> a PresentCompleteNotify event is sent to the client with
> the PresentCompleteModeSuboptimalCopy mode.
> 
> v4: It replaces the old mechanism for format renegotiation

This is v1 of a new patch, not v4 of an existing patch.


> @@ -177,7 +178,12 @@ present_check_flip(RRCrtcPtrcrtc,
>  }
>  
>  /* Ask the driver for permission */
> -if (screen_priv->info->check_flip) {
> +if (screen_priv->info->check_flip2 && reason) {

The check_flip2 field can only be used if (screen_priv->info->version >= 1).


> @@ -564,6 +570,7 @@ present_check_flip_window (WindowPtr window)
>  present_window_priv_ptr window_priv = present_window_priv(window);
>  present_vblank_ptr  flip_pending = screen_priv->flip_pending;
>  present_vblank_ptr  vblank;
> +int reason = 0;

This variable doesn't really need to be initialized, does it?


> @@ -756,10 +764,14 @@ present_execute(present_vblank_ptr vblank, uint64_t 
> ust, uint64_t crtc_msc)
>  /* Compute correct CompleteMode
>   */
>  if (vblank->kind == PresentCompleteKindPixmap) {
> -if (vblank->pixmap && vblank->window)
> -mode = PresentCompleteModeCopy;
> -else
> +if (vblank->pixmap && vblank->window) {
> +if (vblank->reason == PresentFlipReasonBufferFormat)
> +mode = PresentCompleteModeSuboptimalCopy;

Setting PresentCompleteModeSuboptimalCopy will break clients which don't
know about it. There needs to be some kind of handshake to know that the
client can handle it. E.g. via a new PresentOption, or via the minor
version passed in by the client in the PresentQueryVersion request.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver v4 12/14] glamor: Implement GetSupportedModifiers

[Michel Dänzer]

On 16/10/17 07:02 AM, Louis-Francis Ratté-Boulianne wrote:
> Implement function added in DRI3 v1.1.
> 
> A newest version of libepoxy (>= 1.4.4) is required as earlier
> versions use a problematic version of Khronos
> EXT_image_dma_buf_import_modifiers spec.

It might be better to split the Present, modesetting and Xwayland
changes into separate patches.


> v4: Only send scanout-supported modifiers if flipping is possible

What's the reason for that? Scanout compatible tiling is probably
better than linear even if the buffer isn't scanned out directly,
isn't it?


> +_X_EXPORT Bool
> +glamor_get_drawable_modifiers(DrawablePtr draw, CARD32 format,
> +  CARD32 *num_modifiers, uint64_t **modifiers)
> +{
> +struct glamor_screen_private *glamor_priv =
> +glamor_get_screen_private(draw->pScreen);
> +
> +if (glamor_priv->get_drawable_modifiers) {
> +return glamor_priv->get_drawable_modifiers(draw, format,
> +   num_modifiers, modifiers);
> +}
> +*num_modifiers = 0;
> +return TRUE;
> +}

I think the logic would be clearer as:

if (!glamor_priv->get_drawable_modifiers) {
*num_modifiers = 0;
return TRUE;
}

return glamor_priv->get_drawable_modifiers(draw, format,
   num_modifiers, modifiers);
}


> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index cb1f03dc9..4e60569a1 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -488,6 +488,95 @@ glamor_pixmap_from_fds(ScreenPtr screen,
>  return pixmap;
>  }
>  
> +_X_EXPORT Bool
> +glamor_get_formats(ScreenPtr screen,
> +   CARD32 *num_formats, CARD32 **formats)
> +{
> +#ifdef GLAMOR_HAS_EGL_QUERY_DMABUF
> +struct glamor_egl_screen_private *glamor_egl;
> +EGLint num;
> +
> +glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
> +
> +if (!glamor_egl->dmabuf_capable)
> +return FALSE;
> +
> +if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, 0, NULL, )) {
> +*num_formats = 0;
> +return FALSE;
> +}
> +
> +if (num == 0) {
> +*num_formats = 0;
> +return TRUE;
> +}
> +
> +*formats = calloc(num, sizeof(CARD32));
> +if (*formats == NULL) {
> +*num_formats = 0;
> +return FALSE;
> +}
> +
> +if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, num,
> +  (EGLint *) *formats, )) {
> +*num_formats = 0;
> +free(*formats);
> +return FALSE;
> +}
> +
> +*num_formats = num;
> +return TRUE;
> +#else
> +*num_formats = 0;
> +return TRUE;
> +#endif
> +}

Seems weird to return TRUE from glamor_get_formats/modifiers when
GLAMOR_HAS_EGL_QUERY_DMABUF isn't defined. It would probably be better
not to set those hooks in dri3_screen_info_rec in the first place in that
case.


> diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c
> index 3053e53f1..f11c545e2 100644
> --- a/hw/xfree86/common/xf86Mode.c
> +++ b/hw/xfree86/common/xf86Mode.c
> @@ -86,6 +86,7 @@
>  
>  #include 
>  #include "xf86Modes.h"
> +#include "xf86Crtc.h"
>  #include "os.h"
>  #include "servermd.h"
>  #include "globals.h"

It's odd to add this #include without any other changes in this file.
Why is it needed?


> diff --git a/present/present.c b/present/present.c
> index 4d38d2d84..3efae348b 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -611,6 +611,44 @@ present_check_flip_window (WindowPtr window)
>  }
>  }
>  
> +Bool
> +present_can_window_flip(WindowPtr window)
> +{
> +ScreenPtr   screen = window->drawable.pScreen;
> +PixmapPtr   window_pixmap;
> +WindowPtr   root = screen->root;
> +present_screen_priv_ptr screen_priv = present_screen_priv(screen);
> +
> +if (!screen_priv)
> +return FALSE;
> +
> +if (!screen_priv->info)
> +return FALSE;
> +
> +/* Check to see if the driver supports flips at all */
> +if (!screen_priv->info->flip)
> +return FALSE;
> +
> +/* Make sure the window hasn't been redirected with Composite */
> +window_pixmap = screen->GetWindowPixmap(window);
> +if (window_pixmap != screen->GetScreenPixmap(screen) &&
> +window_pixmap != screen_priv->flip_pixmap &&
> +window_pixmap != present_flip_pending_pixmap(screen))
> +return FALSE;

Could short-cut this a little if the window is already flipping:

window_pixmap = screen->GetWindowPixmap(window);
if (window_pixmap == screen_priv->flip_pixmap ||
window_pixmap == present_flip_pending_pixmap(screen))
return TRUE;

if (window_pixmap != screen->GetScreenPixmap(screen))
return FALSE;

[...]


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X 

Re: VK_EXT_aquire_xlib_display and kernel security concerns

[James Jones]

On 10/16/2017 05:49 PM, Keith Packard wrote:

Andy Ritger  writes:


If the NVIDIA X driver finds an HMD display, it:

(a) Marks it as disconnected.
(b) Does not make its EDID available to RandR clients.

So, unless I'm mistaken, RandR clients will see the HMD as an RandR
output.  But, perhaps the problem is that the RandR client cannot tell
that the output is an HMD, since the EDID is not available?


It will look like a regular disconnected output...


Item (b) was only an artifact of how the code is structured, not an
intentional decision.  To be fair, disconnected output with EDID sounds
like a slightly odd state.


Well, the goal is to have regular X clients ignore the output, for which
reporting a Disconnected state seems the best way. The only question is
then how to report to 'special' clients that there is an HMD present on
the connector. We could:

  a) Create a new request to query 'hidden' outputs
  b) Report the 'connected' state in a new output property
  c) Let the client infer that a valid EDID indicates that
 a display is connected.

We would still send RROutputChangeNotify events when the device was
connected/disconnected so that applications could track the state of the
device, but of course the 'connection' status in that event would always
be Disconnected.


Anyway, we can update the NVIDIA driver behavior to match whatever
consensus we reach here.


Let's try to poke holes in the simple plan that Dave suggested above; I
can't see any offhand, but I haven't tried to implement it in the X
server or applications either.

Btw, was there any particular reason the acquire_xlib_display extension
used Xlib types instead of xcb types?


That's just what was asked for IIRC.  There could be a 
VK_EXT_acquire_xcb_display, VK_EXT_acquire_wayland_display, 
VK_EXT_acquire_win32_display, etc. as demand dictates.


Thanks,
-James
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Present extension changes needed for GL/Vulkan vblank modes

[Michel Dänzer]

On 17/10/17 10:48 AM, Michel Dänzer wrote:
> On 17/10/17 06:52 AM, Keith Packard wrote:
>>
>> PresentOptionRelative
>>
>> If 'options' contains PresentOptionRelative, then the most
>> recent actual MSC for a PresentPixmap request to the same
>> drawable (i.e., the MSC value that would have been reported in
>> the associated PresentCompleteNotify event) will be added to
>> [apparent copy-paste accident snipped]
>> target-msc.
>>
>> If no PresentPixmap request has been made to this drawable, then
>> the current MSC value for the drawable will be added to
>> target-msc.
> 
> I see an issue with basing the relative target on the last effective
> MSC: Imagine that the application misses a large number of frames, e.g.
> because the user hit Ctrl-Z. This means that the target MSC values will
> trail the drawable's current MSC, and the application's presentation
> timing may be "different" until it catches up.
> 
> 
> How about this instead: If there are pending PresentPixmap requests for
> the window, the relative target will be added to the latest target MSC
> of the pending requests.

Or maybe actual MSC instead of target MSC.

> Otherwise, it will be added to the drawable's current MSC.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Present extension changes needed for GL/Vulkan vblank modes

[Michel Dänzer]

On 17/10/17 06:52 AM, Keith Packard wrote:
> 
> I know, Present was supposed to support all of the GL vblank modes, and
> it mostly does, except for the most common case...
> 
> In any case, here's a summary of four presentation modes from Vulkan,
> how you'd get that with GL and Present and what they mean:
> 
> Vulkan  GL  Present
> 
> IMMEDIATE   glXSwapIntervalEXT(dpy, draw, 0)
> PresentPixmap(target-msc=0,
> glXSwapBuffers(dpy, draw) PresentOptionAsync)
> 
> Display the presented frame immediately, even if
> that makes the screen tear.
> 
> MAILBOX glXSwapBuffersMscOML(dpy, draw, 0,  
> PresentPixmap(target-msc=0)
>   0, 0, NULL, NULL, NULL)
> 
> Display the presented frame at the next vblank
> interval, replacing any frame already queued at
> that time.
> 
> FIFOglXSwapIntervalEXT(dpy, draw, 1)
> PresentPixmap(target-msc=previous-msc+1)
> glXSwapBuffers(dpy, draw)
> 
> Place the frame to be presented at the end of a
> queue. At each vblank interval, if the queue is
> non-empty, display the first member of the
> queue.
> 
> 
> FIFO_RELAXEDglXSwapIntervalEXT(dpy, draw, -1)   
> PresentPixmap(target-msc=previous-msc+1,
> glXSwapBuffers(dpy, draw)  PresentOptionAsync)
> 
> Place the frame to be presented at the end of a
> queue. At each vblank interval, if the queue is
> non-empty, display the first member of the
> queue. Additionally, if the queue was empty and
> no frame switch happened for this vblank
> interval, immediately switch to the new frame
> 
> From the X protocol perspective, I think we're all set for IMMEDIATE and
> MAILBOX.

However, the current implementation cannot do true MAILBOX behaviour,
because it cannot replace a frame which is already queued in the kernel.


> However, I think both FIFO and FIFO_RELAXED cannot be done
> because they rely on the client knowing the previous MSC (Media Stream
> Counter, or frame counter) value, which the client really isn't going to
> know unless it's presenting every frame.
> 
> To fix them, I think we need to add relative MSC values to Present,
> so we can ask that the X server do the right thing.
> 
> PresentOptionRelative
> 
> If 'options' contains PresentOptionRelative, then the most
> recent actual MSC for a PresentPixmap request to the same
> drawable (i.e., the MSC value that would have been reported in
> the associated PresentCompleteNotify event) will be added to
> [apparent copy-paste accident snipped]
> target-msc.
> 
> If no PresentPixmap request has been made to this drawable, then
> the current MSC value for the drawable will be added to
> target-msc.

I see an issue with basing the relative target on the last effective
MSC: Imagine that the application misses a large number of frames, e.g.
because the user hit Ctrl-Z. This means that the target MSC values will
trail the drawable's current MSC, and the application's presentation
timing may be "different" until it catches up.


How about this instead: If there are pending PresentPixmap requests for
the window, the relative target will be added to the latest target MSC
of the pending requests. Otherwise, it will be added to the drawable's
current MSC.


Other than that, I think the PresentOptionRelative flag makes sense.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: VK_EXT_aquire_xlib_display and kernel security concerns

[Daniel Vetter]

Just throwing a bit more context into the discussion here.

On Mon, Oct 16, 2017 at 11:51 PM, James Jones  wrote:
> On 10/16/2017 01:33 PM, Dave Airlie wrote:
>>
>> On 17 October 2017 at 06:01, James Jones  wrote:
>>>
>>> On 10/16/2017 12:28 PM, Keith Packard wrote:


 James Jones  writes:

> I think at a lower level, we have different views of how
> vkGetPhysicalDeviceDisplayPropertiesKHR/VK_KHR_display works.
> vkGetPhysicalDeviceDisplayPropertiesKHR() is suppose to enumerate all
> the displays on a given device.



 In many devices, the display controller and rendering hardware are
 separate devices, so it's up to user space to figure out the connection
 between them, and user DMAbufs to pass rendered output to the display.
>>>
>>>
>>>
>>> Understood.  In that case, no displays should be enumerated on the
>>> "rendering hardware" device in Vulkan, unless the driver vendor opts for
>>> heroics.
>>>
> I assumed DRM drivers would try the display-capable node, then fall
> back to a render node if that failed.



 No, they cannot as the display node is protected from normal user
 access.
>>>
>>>
>>>
>>> Right, but normal processes don't need the display node, or aren't
>>> supposed
>>> to.  I assumed apps that need direct-to-display would have elevated
>>> permissions on systems configured to require it, or use "acquire"
>>> extensions, potentially combined with native secondary auth mechanisms,
>>> to
>>> get access.  This is why it bothers me that "normal user access" doesn't
>>> include display enumeration.
>>
>>
>> If they use acquire extensions they get display enumeration, the problem
>> is
>> display enumeration before the acquire would need special permission
>> elevation.
>>
> VK_KHR_display, but the core of it is really just a display
> enumeration API.



 Right, I think all I really need is to hand the driver a connection to
 the X server and it can use that to enumerate the available displays
 through the Vulkan API.
>>
>>
>> The thing is if you are running under X I don't think apps should be
>> bypassing
>> things and going straight to hw enumeration, it makes too many problems
>> harder,
>> and in systems where all you have are separated display/render devices it
>> makes this extension impossible to implement, since as you've said vulkan
>> has
>> no way to enumerate such things.
>>>
>>>

>>>
>>
>>>
>>> Given the current definition of a Vulkan-capable device, yes.  It's
>>> annoying
>>> that Vulkan can't yet expose a device that only has displays and the
>>> ability
>>> to import memory objects and create images from them.
>>>
>>> I prefer the enumeration ability as a general principle though.  Is the
>>> discussion with Daniel Vetter captured somewhere so I can read up on the
>>> objections?  I couldn't find it with a naive Google or DRI devel search.
>>
>>
>> Since we have to actually publish our kernel APIs people find inventive
>> ways
>> to do things with them that we don't end up breaking later, and having to
>> find
>> ways to keep their stupid working. You don't have this problem, nobody
>> opens
>> /dev/nvidia directly from an app and does anything useful, and even if
>> they do
>> you guys don't care.
>>
>> Previously for example we opened up DRM_WAIT_VBLANK or forgot to close it,
>> this leds to apps directly poking it behind the X server back, trying
>> to determine
>> timings outside the compositor incorrectly.
>>
>> So if we expose all the enumeration APIs to "render" only nodes, then we
>> will
>> get information leaks, like the currently attached framebuffer for
>> planes, ways to
>> poll state like device connectedness, (we get people trying to bypass dpms
>> all
>> the time), able to pull the device properties out, it's just a genie
>> in a bottle scenario
>> once we provide it we can't repeal it later. It's only sane that we
>> restrict the API
>> to what we want to support.
>
>
> Thanks for the background Dave.  This is a good point.  If the granularity
> of current ioctls makes it difficult to draw the right line, would proposing
> new ioctls that can only query non-sensitive state (TBD what that is) be the
> right direction?

The problem isn't so much the lack of ioctls, but additional
information (which the kernel simply doesn't have). On upstream you
essentially have 2 types of drm fds: rendernodes, for rendering, and
master nodes, for display. Buffer sharing between them is done with
dma-buf. They can happen to be provided by the same driver, but you
always have to deal with both (at least if we ignore some older buffer
sharing schemes with some not-so-great security properties).

So for a vk app trying to display on an X11 display, just asking the
kernel directly doesn't make sense, it needs to go through the
compositor (which then knows which display comes from which display fd
it has,