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

[Keith Packard]

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.

> 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.

>> +   /* 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.

> Everything else looks good to me.

Awesome!

Here's an updated version of this patch:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-vulkan-Add-EXT_acquire_xlib_display-v4.patch
Type: text/x-diff
Size: 24798 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180614/d20e5010/attachment-0001.patch>
-------------- next part --------------

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180614/d20e5010/attachment-0001.sig>