[Mesa-dev] [PATCH mesa 7/9] vulkan: Add EXT_acquire_xlib_display [v3]

[Jason Ekstrand]

On Thu, Jun 14, 2018 at 12:24 PM, Keith Packard <keithp at keithp.com> wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> >> Signed-off-by: Keith Packard <keithp at keithp.com>
> >>
> >> fixup for acquire
> >>
> >> fixup for RROutput type
> >>
> >> Signed-off-by: Keith Packard <keithp at keithp.com>
> >>
> >> fixup
> >>
> >
> > Lots of "fixup".  Did you mean to actually comment on what that was?
>
> Sorry; I was squashing patches and moving comments into the main message
> and just left this noise below.
>
> >> +static bool
> >> +wsi_display_mode_matches_x(struct wsi_display_mode *wsi,
> >> +                           xcb_randr_mode_info_t *xcb)
> >> +{
> >> +   return wsi->clock == (xcb->dot_clock + 500) / 1000 &&
> >> +      wsi->hdisplay == xcb->width &&
> >> +      wsi->hsync_start == xcb->hsync_start &&
> >> +      wsi->hsync_end == xcb->hsync_end &&
> >> +      wsi->htotal == xcb->htotal &&
> >> +      wsi->hskew == xcb->hskew &&
> >> +      wsi->vdisplay == xcb->height &&
> >> +      wsi->vsync_start == xcb->vsync_start &&
> >> +      wsi->vsync_end == xcb->vsync_end &&
> >> +      wsi->vtotal == xcb->vtotal &&
> >>
> >
> > You're not checking vscan here.
>
> Yeah, I'm really unsure what vscan means exactly. X only has the
> DOUBLE_SCAN flag, while vscan appears more flexible. DRM drivers appear
> to use vscan == 0 to mean the same thing as single scan mode, which
> seems like it is also covered by vscan == 1. I think what I probably
> need is a function which returns the effective vscan value for both X
> and DRM modes and then compare those. Maybe something like:
>
> static int
> wsi_display_drm_vscan(drmModeModeInfoPtr drm)
> {
>         if (drm->vscan > 1)
>                 return drm->vscan;
>         return 1;
> }
>
> static int
> wsi_display_x_vscan(xcb_randr_mode_info_t *x_mode)
> {
>    if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_SCAN)
>       return 2;
>    return 1;
> }
>
> If these look reasonable, then I could use them as appropriate and the
> values should all compare correctly.
>

I had a chat with one of our kernel display people and he seemed to think
that neither vscan nor doublescan were still a thing on modern display
technologies.  I also did some searching through the DRM code in the kernel
and, as far as I can tell, there are is no code which creates a mode with
vscan != 0.  How would you feel about just rejecting any modes we get in
from KMS which have vscan > 1 or the DBLSCAN flag set and rejecting
anything from XRandR with DOUBLE_SCAN set?  Then we could just delete vscan
from struct wsi_display_mode.  If someone comes back with an HMD or some
other application of the Vulkan display extensions that they want to work
on a doubldscan display, we can deal with it then.

My primary fear here is that I don't want to get a mode from the X server
and try and conjure up some value for vscan and then get it wrong in such a
way that KMS rejects it when we try to flip an image to the display.  It's
better to just reject those modes outright and have
vkGetDisplayModePropertiesKHR return zero modes than to return modes that
don't work.

Does that sound reasonable?


> > Why are you fetching these here and not lower down?  The only uses of
> them
> > inside the "if (!connector)" is to free them.  Seems to be a bit of a
> > waste.
>
> Good point. I've moved them below that block, just above the code which
> uses their values.
>

Much better. Thanks!


> >> +   /* XXX no support for multiple leases yet */
> >> +   if (wsi->fd >= 0)
> >> +      return VK_ERROR_OUT_OF_DATE_KHR;
> >>
> >
> > This function is supposed to return either VK_SUCCESS or
> > VK_ERROR_INITIALIZATION_FAILED.  The errors here and below should
> probably
> > all be VK_ERROR_INITIALIZATION_FAILED.
>
> Thanks. Vulkan error values remain largely a mystery to me. I've changed
> the values.
>

They are largely mysterious in general. :-)  VK_ERROR_OUT_OF_DATE_KHR
exists as an error code to be returned by swapchain functions to indicate
that something has gone sideways and you need a new swapchain before you
can do anything more.  This function, however, does not involve a swapchain
and is really more of an initialization function.  Also, the spec has a
list of allowed return values for each function; it's generally good to
pick from that list. :-)

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180614/5ad25a01/attachment-0001.html>