Message-ID: <CAKMK7uFTpbomjubFYK9cGizwRgEChwGZtntfgmafjAM1zaz88g@mail.gmail.com>
Date:   Thu, 6 Jul 2017 23:49:13 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Keith Packard <keithp@...thp.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dave Airlie <airlied@...hat.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls

On Thu, Jul 6, 2017 at 6:27 PM, Keith Packard <keithp@...thp.com> wrote:
> Daniel Vetter <daniel@...ll.ch> writes:
>
>> I very much like this since the old ioctl really is a rather bad horror
>> show. And since it's tied in with ums drivers everything is
>> complicated.
>
> Thanks for your kind words.
>
>> I started a discussion a while back whether these should be restricted to
>> DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
>> Since it's read-only I guess we can keep it accessible to everyone, but it
>> has a bit the problem that client app developers see this, think it does
>> what it does and then use it to schedule frames without asking the
>> compositor. Which sometimes even works, but isn't really proper design.
>> The reasons seems to be that on X11 there's no EGL extension for accurate
>> timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
>> uncool or something like that).
>
> In the absence of a suitable EGL api, I'm not sure what to suggest,
> other than fixing EGL instead of blaming the kernel...
>
> However, for the leasing stuff, this doesn't really matter as I've got a
> master FD to play with, so if you wanted to restrict it to master,
> that'd be fine by me.

Was just a thought, I don't mind either way really I think.

>>> +
>>> +/*
>>> + * Get crtc VBLANK count.
>>> + *
>>> + * \param dev DRM device
>>> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
>>> + * \param file_priv drm file private for the user's open file descriptor
>>> + */
>>
>> Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
>> comments completely. Someday maybe someone even gets around to doing
>> proper uabi documentation :-) Just an aside.
>
> I'm just trying to follow along with the local "conventions" in the
> file. Let me know if you have a future plan to make this better and I'll
> just reformat to suit.

Yeah that \param stuff is all back from really old libdrm. Just shows
how old this code is :-)

>>> +
>>> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>> +                            struct drm_file *file_priv)
>>> +{
>>> +    struct drm_crtc *crtc;
>>> +    int pipe;
>>> +    struct drm_crtc_get_sequence *get_seq = data;
>>> +    struct timespec now;
>>> +
>>
>> You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
>> below.
>
> Like this?

Yup.

>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index e39b2bd074e4..3738ff484f36 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1712,6 +1712,9 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>         struct drm_crtc_get_sequence *get_seq = data;
>         struct timespec now;
>
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +               return -EINVAL;
> +
>         if (!dev->irq_enabled)
>                 return -EINVAL;
>
> @@ -1749,6 +1752,9 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>         int ret;
>         unsigned long spin_flags;
>
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +               return -EINVAL;
> +
>         if (!dev->irq_enabled)
>                 return -EINVAL;
>
>
>>> +    get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>>
>> This can give you and old vblank if the vblank is off (i.e. sw state
>> hasn't be regularly updated). I think we want a new
>> drm_crtc_accurate_vblank_count_and_time variant.
>
> Right, I saw that code in the wait_vblank case and forgot to carry it
> over. Here's a duplicate of what that function does; we'll need this
> code in any case for drivers which don't provide the necessary support
> for  accurate vblank counts:
>
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1711,6 +1711,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>         int pipe;
>         struct drm_crtc_get_sequence *get_seq = data;
>         struct timespec now;
> +       bool vblank_enabled;
> +       int ret;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -1724,8 +1726,19 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>
>         pipe = drm_crtc_index(crtc);
>
> +       vblank_enabled = dev->vblank_disable_immediately && READ_ONCE(vblank->enabled);
> +
> +       if (!vblank_enabled) {
> +               ret = drm_vblank_get(dev, pipe);
> +               if (ret) {
> +                       DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> +                       return ret;
> +               }
> +       }
>         get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>         get_seq->sequence_ns = timespec_to_ns(&now);
> +       if (!vblank_enabled)
> +               drm_vblank_put(dev, pipe);
>         return 0;
>  }

Yeah looks good.

>> Another thing that is very ill-defined in the old vblank ioctl and that we
>> could fix here: Asking for vblanks when the CRTC is off is a bit silly.
>> Asking for the sequence when it's off makes some sense, but might still be
>> good to give userspace some indication in the new struct? This also from
>> the pov of the unpriviledge vblank waiter use-case that I wondered about
>> earlier.
>
> Hrm. It's certainly easy to do, however an application using this
> without knowing the enabled state of the crtc is probably not doing the
> right thing...
>
> Here's what that looks like, I think, in case we want to do this:

Yeah the annoying bit is that we have to grab the mutex. I think
better to postpone this (we can always add a flag) and only do it when
there's a real need. Was just an idea that might be useful.

> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1735,6 +1735,12 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>                         return ret;
>                 }
>         }
> +       drm_modeset_lock(&crtc->mutex, NULL);
> +       if (crtc->state)
> +               get_seq->active = crtc->state->enable;
> +       else
> +               get_seq->active = crtc->enabled;
> +       drm_modeset_unlock(&crtc->mutex);
>         get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>         get_seq->sequence_ns = timespec_to_ns(&now);
>         if (!vblank_enabled)
>
>>> +    /* Check for valid signal edge */
>>> +    if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
>>> +            return -EINVAL;
>>
>> This seems new, maybe drop it until we need it?
>
> It's part of Vulkan, so I want to expose it in the kernel API. And,
> making sure user-space isn't setting unexpected bits seems like a good
> idea.

So the event we generate should be for the very first pixel iirc, or
top of frame or whatever OML_sync says, so it should match vulkan I
think. I just meant we can remove the #define since it's rejected
anyway (we reject all unknown flags), and we can easily add it later
on.

>> drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
>> pairs as much as possible. Same for all the others.
>
> Sure. I'll note that this is just a wrapper around drm_vblank_get/put at
> this point :-)

Yeah I'm not there yet :-)

>> I think here there's no need for the accurate version, since we
>> force-enable the vblanks already.
>
> Agreed.
>
>>> +    e->pipe = pipe;
>>> +    e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
>>> +    e->event.base.length = sizeof(e->event.seq);
>>> +    e->event.seq.user_data = queue_seq->user_data;
>>
>> No need for crtc_id in this event? Or do we expect userspace to encode
>> that in the user_data somehow?
>
> I feared changing the size of the event and so I left that out. And,
> yes, userspace will almost certainly encode a pointer in the user_data,
> which can include whatever information it needs.

I think the size should be a problem, since the old vblank ioctl uses
sizeof(e->event.vbl), extending event.seq shouldn't be a problem. But
we can also postpone this, since iirc we've done the events correctly
and you can extend them at the end.

>> But might be useful just for consistency.
>
> This would require making the event larger, which seems like a bad idea...
>
>>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT   0x00000004      /* Signal when first pixel is displayed */
>>
>> I thought this is already the semantics our current vblank events have (in
>> the timestamp, when exactly the event comes out isn't defined further than
>> "somewhere around vblank"). Since it's unsed I'd just remove this
>> #define.
>
> Vulkan has this single define, but may have more in the future so I
> wanted to make sure the application was able to tell if the kernel
> supported new modes if/when they appear. Reliably returning -EINVAL
> today when the application asks for something which isn't supported
> seems like good practice.

As long as we reject any noise in unused bits/members we can extend
them. No need to have an explicit #define to reject a special bit.

>> In both ioctl handlers pls make sure everything you don't look at is 0,
>> including unused stuff like pad. Otherwise userspace might fail to clear
>> them, and we can never use them in the future. Maybe just rename pad to
>> flags right away.
>
> Good idea. Above, you asked me to return whether the crtc was active in
> the get_sequence ioctl; I suggested putting that in the existing pad
> field, which would leave the whole structure defined.
>
> I've got tiny patches for each piece which I've stuck on my
> drm-sequence-64 branch at
>
>         git://people.freedesktop.org/~keithp/linux.git drm-sequence-64
>
> Most of those are included above, with the exception of the
> drm_crtc_vblank_get/put changes.

tbh that "is the crtc active" was just an idea, I think if you don't
have an immediate use for it in the vk extension we can leave it for
later.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Powered by blists - more mailing lists