[Mesa-dev] [PATCH mesa 07/21] vulkan: Add EXT_acquire_xlib_display [v2]

[Keith Packard]

Jason Ekstrand <jason at jlekstrand.net> writes:

> As with patch 1, I've gone through and made a pile of style changes to
> bring things back under 80 characters.  You can find it on this
> branch:

I've adopted that style across the whole series.

>>  struct wsi_display {
>> @@ -1414,5 +1421,468 @@ wsi_release_display(VkPhysicalDevice
>> physical_device,
>>        close(wsi->fd);
>>        wsi->fd = -1;
>>     }
>> +#ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
>> +   wsi_display_connector_from_handle(display)->output = None;
>>
>
> Does this function ever return NULL?

That's a standard handle to pointer macro -- if display is valid, then
the macro will return a valid pointer.

>> +#ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
>> +
>> +static struct wsi_display_connector *
>> +wsi_display_find_output(struct wsi_device               *wsi_device,
>> +                        RROutput                        output)
>>
>
> You're using RROutput here but xcb_rander_output_t elsewhere.  Why not just
> pick one?

The API is specified using Xlib types, but the implementation uses
XCB. They are the "same" underlying type of course; I have switched to
use the xcb types uniformly in the implementation except in the public
API which then casts from RROutput to xcb_randr_output_t.

> Things are going way over the usual 80 character limit.  I'm fixing up
> whitespace as I read and will put a fixup in my branch.

All fixed.

>> +      xcb_randr_query_version_reply_t           *qv_r =
>> xcb_randr_query_version_reply(connection, qv_c, NULL);
>> +      free(qv_r);
>>
>
> What's up with the version query?  We're throwing it away without ever
> using it.  Does this provide us with some error checkint of some sort?

You're required to call the query_version request before doing any other
RandR requests, and this function may well be making the very first
calls to RandR.

In this case, we're about to call a RandR function supported since time
immemorial, so we don't actually care what version the server
reports. You'll note that we *do* check the version before we start
trying to perform any leasing operations.

>> +      if (!connector) {
>>
>
> This is always true.
...
> And this can be merged in.

Yeah, I noticed that this got refactored several times and the
conditions became mixed up (not "wrong", but illogical). I've rewritten
it so that it makes sense again.

>> +            for (int o = 0; o < num_outputs; o++)
>> +               if (outputs[o] == output && num_outputs == 1) {
>>
>
> checking for num_outputs == 1 inside the loop is a bit odd.

Thanks. Again, refactoring this code left it looking strange. I've
changed this to:

            if (num_outputs == 1 && outputs[0] == output)
               active_crtc = rc[c];

-- 
-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/20180611/80f130e5/attachment.sig>