<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 14, 2018 at 12:24 PM, Keith Packard <span dir="ltr"><<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
>> Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com">keithp@keithp.com</a>><br>
>><br>
>> fixup for acquire<br>
>><br>
>> fixup for RROutput type<br>
>><br>
>> Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com">keithp@keithp.com</a>><br>
>><br>
>> fixup<br>
>><br>
><br>
> Lots of "fixup".  Did you mean to actually comment on what that was?<br>
<br>
</span>Sorry; I was squashing patches and moving comments into the main message<br>
and just left this noise below.<br>
<span class=""><br>
>> +static bool<br>
>> +wsi_display_mode_matches_x(<wbr>struct wsi_display_mode *wsi,<br>
>> +                           xcb_randr_mode_info_t *xcb)<br>
>> +{<br>
>> +   return wsi->clock == (xcb->dot_clock + 500) / 1000 &&<br>
>> +      wsi->hdisplay == xcb->width &&<br>
>> +      wsi->hsync_start == xcb->hsync_start &&<br>
>> +      wsi->hsync_end == xcb->hsync_end &&<br>
>> +      wsi->htotal == xcb->htotal &&<br>
>> +      wsi->hskew == xcb->hskew &&<br>
>> +      wsi->vdisplay == xcb->height &&<br>
>> +      wsi->vsync_start == xcb->vsync_start &&<br>
>> +      wsi->vsync_end == xcb->vsync_end &&<br>
>> +      wsi->vtotal == xcb->vtotal &&<br>
>><br>
><br>
> You're not checking vscan here.<br>
<br>
</span>Yeah, I'm really unsure what vscan means exactly. X only has the<br>
DOUBLE_SCAN flag, while vscan appears more flexible. DRM drivers appear<br>
to use vscan == 0 to mean the same thing as single scan mode, which<br>
seems like it is also covered by vscan == 1. I think what I probably<br>
need is a function which returns the effective vscan value for both X<br>
and DRM modes and then compare those. Maybe something like:<br>
<br>
static int<br>
wsi_display_drm_vscan(<wbr>drmModeModeInfoPtr drm)<br>
{<br>
        if (drm->vscan > 1)<br>
                return drm->vscan;<br>
        return 1;<br>
}<br>
<br>
static int<br>
wsi_display_x_vscan(xcb_randr_<wbr>mode_info_t *x_mode)<br>
{<br>
<span class="">   if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_<wbr>SCAN)<br>
</span>      return 2;<br>
   return 1;<br>
}<br>
<br>
If these look reasonable, then I could use them as appropriate and the<br>
values should all compare correctly.<span class=""><br></span></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Does that sound reasonable?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Why are you fetching these here and not lower down?  The only uses of them<br>
> inside the "if (!connector)" is to free them.  Seems to be a bit of a<br>
> waste.<br>
<br>
</span>Good point. I've moved them below that block, just above the code which<br>
uses their values.<span class=""><br></span></blockquote><div><br></div><div>Much better. Thanks! <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> +   /* XXX no support for multiple leases yet */<br>
>> +   if (wsi->fd >= 0)<br>
>> +      return VK_ERROR_OUT_OF_DATE_KHR;<br>
>><br>
><br>
> This function is supposed to return either VK_SUCCESS or<br>
> VK_ERROR_INITIALIZATION_<wbr>FAILED.  The errors here and below should probably<br>
> all be VK_ERROR_INITIALIZATION_<wbr>FAILED.<br>
<br>
</span>Thanks. Vulkan error values remain largely a mystery to me. I've changed<br>
the values.<span class=""><br></span></blockquote><div><br></div><div>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. :-)</div><div><br></div><div>--Jason<br></div></div></div></div>