[Mesa-dev] [PATCH mesa 01/21] vulkan: Add KHR_display extension using DRM [v4]

[Keith Packard]

Jason Ekstrand <jason at jlekstrand.net> writes:

> If you move the VK_OUTARRAY_MAKE up higher, both of the "goto bail"s can be
> "return vk_outarray_status(&conn)".  Not a big deal though; what you have
> is probably fine.

I'll leave it then.

>> +            prop->currentStackIndex = stack_index++;
>>
>
> In your branch, this is 0.  Why the change?

I was doing it wrong -- this value shows the stacking order of the
plane within the connector. As I'm only (currently) listing a single
plane per connector, each one is on the top (and bottom) of the stack,
hence the constant value.

>> +            prop->currentDisplay = NULL;
>>
>
> This should probably be VK_NULL_HANDLE

Yeah, compiling on 32-bits caught this with a compiler warning. Fixed.

>> +      vk_outarray_append(&conn, present) {
>> +         *present = available_present_modes[c];
>> +      }
>> +   }
>>
>
> I don't think the array is helpful here.  We can just do a
> vk_outarray_append with FIFO and call it good.  If anything, it's probably
> worse since other present modes will probably be dependent on things such
> as whether or not the driver exposes ASYNC.

Good point. I had listed MAILBOX originally, but until the kernel
actually supports replacing a queued buffer, it's kinda lying to report
that on any DRM-based system, so I removed it.

> This may overflow if the provide a large value that is not UINT64_MAX.  We
> should do something such as
>
> uint64_t current = wsi_get_current_monotonic();
> if (timeout + current < timeout)
>    timeout = UINT64_MAX;
> else
>    timeout += current;

Yeah, they could well pass something ridiculous and we might as well
deal with it correctly. I've adapted similar code from the radv driver
instead of the above though; trusting the compiler to actually treat
integers as a finite field has bitten me in the past.

static uint64_t wsi_rel_to_abs_time(uint64_t rel_time)
{
   uint64_t current_time = wsi_get_current_monotonic();

   /* check for overflow */
   if (rel_time > UINT64_MAX - current_time)
      return UINT64_MAX;

   return current_time + rel_time;
}

> This could be crtc_id && wsi_display_crtc_solo(...)

Yes, but the formatting actually looks a bit cleaner this way to me;
with both tests in the same if, the line is too long and you need to
wrap the expression.

>> +   if (wsi->fd < 0)
>> +      return VK_ERROR_INITIALIZATION_FAILED;
>>
>
> Is this even possible?  I don't think it is.  If it isn't, maybe an assert
> would be better?  If it is, it should be VK_ERROR_OUT_OF_DATE_KHR.

I think it is possible - the application might have released the display
before cleaning up the swapchain. OUT_OF_DATE seems right to me; the
application would need to re-acquire the display and then re-create the
swapchain to get back to working.

>> +            if (result != VK_SUCCESS) {
>> +               image->state = WSI_IMAGE_IDLE;
>>
>
> Would QUEUED be more appropreate here?  wsi_display_setup_connector will
> have returned either OUT_OF_MEMORY or OUT_OF_DATE so it probably doesn't
> matter.  I guess that begs the question, why are we even bothering to mess
> around with the image's state?

QUEUED would mean that the image was still waiting to be displayed, and
I think the correct result here is to abandon attempts to display this
image, so I mark the image as IDLE.


> If ret != 0, we continue on and go around the loop again and we try inside
> the "if (connector->active)" block.  It seems like we can get into an
> infinite loop if drmModeSetCrtc keeps returning -EINVAL.  At some point,
> don't we want ot throw OUT_OF_DATE and bail?

Good catch. The drmModeSetCrtc "should" never return -EINVAL as we're
careful to pick only a valid configuration, but of course there's no
trusting the kernel to not have some other random problem.

The idea is that drmModePageFlip may return -EINVAL if something has
changed in the configuration (like DPMS off, or VT switch), and in that
case (and in the case of having not set a mode yet), we want to do the
mode set instead.

If the mode set fails with EACCES, then we need to wait for the VT to
become active again (done by polling once a second).

If the mode set fails for any other reason, then I think we just want to
give up and report an error back to the client. Something bad happened.

So, now the code looks like this:

        ret = page_flip
        if (ret == 0) {
                image->state = FLIPPING;
                return success
        }

        if (ret == -EINVAL) {
                ret = mode_set
                if (ret == 0) {
                        image->state = DISPLAYING;
                        return success;
                }
        }
        if (ret != -EACCES) {
                give up!

> Again, why are we setting it to IDLE instead of leaving it in QUEUED?  This
> doesn't seem to do anything useful.

It doesn't really matter -- the swapchain is probably broken at this
point anyways. I set it to idle because this way the system doesn't even
bother to try to display it again.

> Why are you initializing this twice?

Just to make sure the bits are really set? I've removed the first case
(as it didn't bother to check the return value).

> Given that you destroy the condattr here, maybe it would make sense to have
> a simple init_ptread_cond_monotonic helper to keep the clean-up path
> simpler?  If someone added any initialization after this that needed a
> cleanup, it would get tricky with the condattr destroy.

Good idea. Done.

> And, there's the end. :-)  Nothing too drastic.  A couple of bugs and a
> suggestion or two.  On to patch 2. :-)

Nice suggestions, and a couple of good bugs.

I'm planning on submitting an MR once I've finished with your review.

-- 
-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/fd05d802/attachment-0001.sig>