Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681906 > unrolled thread

[PATCH 0/3] drm: Add CRTC-id based ioctls for vblank query/event

Started byKeith Packard <keithp@keithp.com>
First post2017-07-06 00:20 +0200
Last post2017-07-07 14:10 +0200
Articles 7 on this page of 27 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] drm: Add CRTC-id based ioctls for vblank query/event Keith Packard <keithp@keithp.com> - 2017-07-06 00:20 +0200
    [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns Keith Packard <keithp@keithp.com> - 2017-07-06 00:20 +0200
      Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Daniel Vetter <daniel@ffwll.ch> - 2017-07-06 09:30 +0200
        Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns Keith Packard <keithp@keithp.com> - 2017-07-06 17:10 +0200
          Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Daniel Vetter <daniel@ffwll.ch> - 2017-07-07 14:20 +0200
      Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Michel Dänzer <michel@daenzer.net> - 2017-07-06 09:50 +0200
        Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Michel Dänzer <michel@daenzer.net> - 2017-07-06 10:10 +0200
          Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns Keith Packard <keithp@keithp.com> - 2017-07-06 17:20 +0200
        Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns Keith Packard <keithp@keithp.com> - 2017-07-06 17:10 +0200
          Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Michel Dänzer <michel@daenzer.net> - 2017-07-07 03:40 +0200
            Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank  time precision to ns Michel Dänzer <michel@daenzer.net> - 2017-07-07 04:10 +0200
    [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Keith Packard <keithp@keithp.com> - 2017-07-06 00:20 +0200
      Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls Daniel Vetter <daniel@ffwll.ch> - 2017-07-06 10:00 +0200
        Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-07-06 12:20 +0200
          Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Daniel Vetter <daniel@ffwll.ch> - 2017-07-06 13:10 +0200
            Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-07-06 16:10 +0200
              Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Keith Packard <keithp@keithp.com> - 2017-07-06 18:30 +0200
                Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-07-06 20:10 +0200
                  Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Keith Packard <keithp@keithp.com> - 2017-07-06 20:30 +0200
                    Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-07-06 21:00 +0200
                      Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Keith Packard <keithp@keithp.com> - 2017-07-06 21:50 +0200
        Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Keith Packard <keithp@keithp.com> - 2017-07-06 18:30 +0200
          Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls Daniel Vetter <daniel@ffwll.ch> - 2017-07-06 23:50 +0200
    [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types Keith Packard <keithp@keithp.com> - 2017-07-06 00:20 +0200
      Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future  event types Daniel Vetter <daniel@ffwll.ch> - 2017-07-06 09:40 +0200
        Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types Keith Packard <keithp@keithp.com> - 2017-07-06 17:40 +0200
          Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future  event types Daniel Vetter <daniel@ffwll.ch> - 2017-07-07 14:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1682676 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls

FromKeith Packard <keithp@keithp.com>
Date2017-07-06 21:50 +0200
SubjectRe: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls
Message-ID<u0kWK-1y4-25@gated-at.bofh.it>
In reply to#1682653

[Multipart message — attachments visible in raw view] — view raw

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> With the disable_immediate thing we only wait until the next vblank
> before disabling the irq again.

Ok, still sounds like we'll be doing fine if the application does a
get immediately followed by a queue event. At least most of the time.

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1682564 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls

FromKeith Packard <keithp@keithp.com>
Date2017-07-06 18:30 +0200
SubjectRe: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls
Message-ID<u0hPc-7Y2-19@gated-at.bofh.it>
In reply to#1682144

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.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.

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

>> +
>> +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?

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;
 }

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

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

> 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 :-)

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

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

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

Thanks much for your careful review.

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1682731 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-07-06 23:50 +0200
SubjectRe: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls
Message-ID<u0mOT-2P4-13@gated-at.bofh.it>
In reply to#1682564
On Thu, Jul 6, 2017 at 6:27 PM, Keith Packard <keithp@keithp.com> wrote:
> Daniel Vetter <daniel@ffwll.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

[toc] | [prev] | [next] | [standalone]

#1681909 — [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types

FromKeith Packard <keithp@keithp.com>
Date2017-07-06 00:20 +0200
Subject[PATCH 2/3] drm: Reorganize drm_pending_event to support future event types
Message-ID<u00Om-3Tl-17@gated-at.bofh.it>
In reply to#1681906
Place drm_event_vblank in a new union that includes that and a bare
drm_event structure. This will allow new members of that union to be
added in the future without changing code related to the existing vbl
event type.

Assignments to the crtc_id field are now done when the event is
allocated, rather than when delievered. This way, delivery doesn't
need to have the crtc ID available.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/drm_atomic.c         |  7 ++++---
 drivers/gpu/drm/drm_plane.c          |  2 +-
 drivers/gpu/drm/drm_vblank.c         | 27 ++++++++++++++++-----------
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  4 ++--
 include/drm/drm_vblank.h             |  8 +++++++-
 6 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c0f336d23f9c..f569d7f03f3c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
  */
 
 static struct drm_pending_vblank_event *create_vblank_event(
-		struct drm_device *dev, uint64_t user_data)
+		struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
 {
 	struct drm_pending_vblank_event *e = NULL;
 
@@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
 
 	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
 	e->event.base.length = sizeof(e->event);
-	e->event.user_data = user_data;
+	e->event.vbl.crtc_id = crtc->base.id;
+	e->event.vbl.user_data = user_data;
 
 	return e;
 }
@@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
 			struct drm_pending_vblank_event *e;
 
-			e = create_vblank_event(dev, arg->user_data);
+			e = create_vblank_event(dev, crtc, arg->user_data);
 			if (!e)
 				return -ENOMEM;
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 5dc8c4350602..fe9f31285bc2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		}
 		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
 		e->event.base.length = sizeof(e->event);
-		e->event.user_data = page_flip->user_data;
+		e->event.vbl.user_data = page_flip->user_data;
 		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
 		if (ret) {
 			kfree(e);
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f55f997c0b8f..9ae170857ef6 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev,
 		struct drm_pending_vblank_event *e,
 		u64 seq, struct timespec *now)
 {
-	e->event.sequence = seq;
-	e->event.tv_sec = now->tv_sec;
-	e->event.tv_usec = now->tv_nsec / 1000;
-
-	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
-					 e->event.sequence);
-
+	switch (e->event.base.type) {
+	case DRM_EVENT_VBLANK:
+	case DRM_EVENT_FLIP_COMPLETE:
+		if (seq)
+			e->event.vbl.sequence = (u32) seq;
+		if (now) {
+			e->event.vbl.tv_sec = now->tv_sec;
+			e->event.vbl.tv_usec = now->tv_nsec / 1000;
+		}
+		break;
+	}
+	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
 	drm_send_event_locked(dev, &e->base);
 }
 
@@ -865,7 +870,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 
 	e->pipe = pipe;
 	e->sequence = drm_vblank_count(dev, pipe);
-	e->event.crtc_id = crtc->base.id;
 	list_add_tail(&e->base.link, &dev->vblank_event_list);
 }
 EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
@@ -897,7 +901,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 		now = get_drm_timestamp();
 	}
 	e->pipe = pipe;
-	e->event.crtc_id = crtc->base.id;
 	send_vblank_event(dev, e, seq, &now);
 }
 EXPORT_SYMBOL(drm_crtc_send_vblank_event);
@@ -1342,6 +1345,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 				  union drm_wait_vblank *vblwait,
 				  struct drm_file *file_priv)
 {
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e;
 	struct timespec now;
@@ -1357,8 +1361,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 
 	e->pipe = pipe;
 	e->event.base.type = DRM_EVENT_VBLANK;
-	e->event.base.length = sizeof(e->event);
-	e->event.user_data = vblwait->request.signal;
+	e->event.base.length = sizeof(e->event.vbl);
+	e->event.vbl.user_data = vblwait->request.signal;
+	e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8d7dc9def7c2..c13b97338310 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
 
 		ret = vmw_event_fence_action_queue(file_priv, fence,
 						   &event->base,
-						   &event->event.tv_sec,
-						   &event->event.tv_usec,
+						   &event->event.vbl.tv_sec,
+						   &event->event.vbl.tv_usec,
 						   true);
 	}
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index bad31bdf09b6..4e329588ce9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
 
 		ret = vmw_event_fence_action_queue(file_priv, fence,
 						   &event->base,
-						   &event->event.tv_sec,
-						   &event->event.tv_usec,
+						   &event->event.vbl.tv_sec,
+						   &event->event.vbl.tv_usec,
 						   true);
 		vmw_fence_obj_unreference(&fence);
 	} else {
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 68e99177fff3..3ef7ddc5db5f 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
 	/**
 	 * @event: Actual event which will be sent to userspace.
 	 */
-	struct drm_event_vblank event;
+	union {
+		struct drm_event base;
+		struct drm_event_vblank vbl;
+	} event;
 };
 
 /**
@@ -163,6 +166,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 			       struct drm_pending_vblank_event *e);
 void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 			      struct drm_pending_vblank_event *e);
+void drm_vblank_set_event(struct drm_pending_vblank_event *e,
+			  u64 *seq,
+			  struct timespec *now);
 bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
 bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
 int drm_crtc_vblank_get(struct drm_crtc *crtc);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1682132 — Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-07-06 09:40 +0200
SubjectRe: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types
Message-ID<u09yi-1lP-19@gated-at.bofh.it>
In reply to#1681909
On Wed, Jul 05, 2017 at 03:10:12PM -0700, Keith Packard wrote:
> Place drm_event_vblank in a new union that includes that and a bare
> drm_event structure. This will allow new members of that union to be
> added in the future without changing code related to the existing vbl
> event type.
> 
> Assignments to the crtc_id field are now done when the event is
> allocated, rather than when delievered. This way, delivery doesn't
> need to have the crtc ID available.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

A few nits below, but looks good otherwise.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c         |  7 ++++---
>  drivers/gpu/drm/drm_plane.c          |  2 +-
>  drivers/gpu/drm/drm_vblank.c         | 27 ++++++++++++++++-----------
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  4 ++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  4 ++--
>  include/drm/drm_vblank.h             |  8 +++++++-
>  6 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..f569d7f03f3c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> -		struct drm_device *dev, uint64_t user_data)
> +		struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)

Nit: Please also drop the dev argument, we have crtc->dev easily
available. That fits better into my long-term goal of getting rid of the
(dev, pipe) pairs everywhere in the vblank code and fully switching over
to drm_crtc *.

>  {
>  	struct drm_pending_vblank_event *e = NULL;
>  
> @@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  
>  	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  	e->event.base.length = sizeof(e->event);
> -	e->event.user_data = user_data;
> +	e->event.vbl.crtc_id = crtc->base.id;
> +	e->event.vbl.user_data = user_data;
>  
>  	return e;
>  }
> @@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
>  		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
>  			struct drm_pending_vblank_event *e;
>  
> -			e = create_vblank_event(dev, arg->user_data);
> +			e = create_vblank_event(dev, crtc, arg->user_data);
>  			if (!e)
>  				return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5dc8c4350602..fe9f31285bc2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		}
>  		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  		e->event.base.length = sizeof(e->event);
> -		e->event.user_data = page_flip->user_data;
> +		e->event.vbl.user_data = page_flip->user_data;
>  		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
>  		if (ret) {
>  			kfree(e);
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f55f997c0b8f..9ae170857ef6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev,
>  		struct drm_pending_vblank_event *e,
>  		u64 seq, struct timespec *now)
>  {
> -	e->event.sequence = seq;
> -	e->event.tv_sec = now->tv_sec;
> -	e->event.tv_usec = now->tv_nsec / 1000;
> -
> -	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
> -					 e->event.sequence);
> -
> +	switch (e->event.base.type) {
> +	case DRM_EVENT_VBLANK:
> +	case DRM_EVENT_FLIP_COMPLETE:
> +		if (seq)
> +			e->event.vbl.sequence = (u32) seq;
> +		if (now) {
> +			e->event.vbl.tv_sec = now->tv_sec;
> +			e->event.vbl.tv_usec = now->tv_nsec / 1000;
> +		}
> +		break;
> +	}

Not sure why this change? Also prep for the new, presumably extended
events? Seems at least slightly inconsistent with other paths, where we
still unconditionally fill it in.

> +	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>  	drm_send_event_locked(dev, &e->base);
>  }
>  
> @@ -865,7 +870,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  
>  	e->pipe = pipe;
>  	e->sequence = drm_vblank_count(dev, pipe);
> -	e->event.crtc_id = crtc->base.id;
>  	list_add_tail(&e->base.link, &dev->vblank_event_list);
>  }
>  EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> @@ -897,7 +901,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> -	e->event.crtc_id = crtc->base.id;
>  	send_vblank_event(dev, e, seq, &now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> @@ -1342,6 +1345,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  				  union drm_wait_vblank *vblwait,
>  				  struct drm_file *file_priv)
>  {
> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);

This'll oops on ums drivers since kms isn't set up. And we can call this
from ums drivers in the old vblank ioctl. My long-term goal is to
completely separate these two worlds, and exclusive deal with drm_crtc *
in the kms side (and only compute the pipe index when needed). But we're
not there yet. I also want to split it into two files (drm_crtc_vblank.c
and drm_legacy_vblank.c) and make sure we consistently use
drm_crtc_vblank_ as the new-world prefix.

Probably the simplest option is to extend this to take all three (dev,
pipe, crtc) as arguments, and then pass NULL for the old vblank ioctl, and
the right pointer for the new stuff. Or you stuff a DRIVER_MODESET check
at the right spot somewhere.

Or maybe I shouldn't have told you this and seized this opportunity to
break all the old drivers :-)

>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
>  	struct timespec now;
> @@ -1357,8 +1361,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  
>  	e->pipe = pipe;
>  	e->event.base.type = DRM_EVENT_VBLANK;
> -	e->event.base.length = sizeof(e->event);
> -	e->event.user_data = vblwait->request.signal;
> +	e->event.base.length = sizeof(e->event.vbl);
> +	e->event.vbl.user_data = vblwait->request.signal;
> +	e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;

>  
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 8d7dc9def7c2..c13b97338310 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>  
>  		ret = vmw_event_fence_action_queue(file_priv, fence,
>  						   &event->base,
> -						   &event->event.tv_sec,
> -						   &event->event.tv_usec,
> +						   &event->event.vbl.tv_sec,
> +						   &event->event.vbl.tv_usec,
>  						   true);
>  	}
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index bad31bdf09b6..4e329588ce9c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
>  
>  		ret = vmw_event_fence_action_queue(file_priv, fence,
>  						   &event->base,
> -						   &event->event.tv_sec,
> -						   &event->event.tv_usec,
> +						   &event->event.vbl.tv_sec,
> +						   &event->event.vbl.tv_usec,
>  						   true);
>  		vmw_fence_obj_unreference(&fence);
>  	} else {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 68e99177fff3..3ef7ddc5db5f 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
>  	/**
>  	 * @event: Actual event which will be sent to userspace.
>  	 */
> -	struct drm_event_vblank event;
> +	union {
> +		struct drm_event base;
> +		struct drm_event_vblank vbl;
> +	} event;
>  };
>  
>  /**
> @@ -163,6 +166,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  			       struct drm_pending_vblank_event *e);
>  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  			      struct drm_pending_vblank_event *e);
> +void drm_vblank_set_event(struct drm_pending_vblank_event *e,
> +			  u64 *seq,
> +			  struct timespec *now);
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1682519 — Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types

FromKeith Packard <keithp@keithp.com>
Date2017-07-06 17:40 +0200
SubjectRe: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types
Message-ID<u0h2N-73E-5@gated-at.bofh.it>
In reply to#1682132

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> A few nits below, but looks good otherwise.

Thanks.

>>  static struct drm_pending_vblank_event *create_vblank_event(
>> -		struct drm_device *dev, uint64_t user_data)
>> +		struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
>
> Nit: Please also drop the dev argument, we have crtc->dev easily
> available. That fits better into my long-term goal of getting rid of the
> (dev, pipe) pairs everywhere in the vblank code and fully switching over
> to drm_crtc *.

As 'dev' isn't used anyways, this seems like a fine plan.

>> +	switch (e->event.base.type) {
>> +	case DRM_EVENT_VBLANK:
>> +	case DRM_EVENT_FLIP_COMPLETE:
>> +		if (seq)
>> +			e->event.vbl.sequence = (u32) seq;
>> +		if (now) {
>> +			e->event.vbl.tv_sec = now->tv_sec;
>> +			e->event.vbl.tv_usec = now->tv_nsec / 1000;
>> +		}
>> +		break;
>> +	}
>
> Not sure why this change? Also prep for the new, presumably extended
> events? Seems at least slightly inconsistent with other paths, where we
> still unconditionally fill it in.

Yes, this prepares for the new events to make that patch smaller. The
places where the data are still unconditionally assigned should know
that the event in the struct is either a VBLANK or FLIP_COMPLETE.

>> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>
> This'll oops on ums drivers since kms isn't set up.

How about this fix?

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 857b7cf011e1..e39b2bd074e4 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 				  union drm_wait_vblank *vblwait,
 				  struct drm_file *file_priv)
 {
-	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e;
 	struct timespec now;
@@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	e->event.base.type = DRM_EVENT_VBLANK;
 	e->event.base.length = sizeof(e->event.vbl);
 	e->event.vbl.user_data = vblwait->request.signal;
-	e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
+	e->event.vbl.crtc_id = 0;
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+		if (crtc)
+			e->event.vbl.crtc_id = crtc->base.id;
+	}
 
 	spin_lock_irqsave(&dev->event_lock, flags);

> Or maybe I shouldn't have told you this and seized this opportunity to
> break all the old drivers :-)

You now know my evil plan :-)

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1683151 — Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-07-07 14:10 +0200
SubjectRe: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types
Message-ID<u0Af8-3Q5-13@gated-at.bofh.it>
In reply to#1682519
On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > A few nits below, but looks good otherwise.
> 
> Thanks.
> 
> >>  static struct drm_pending_vblank_event *create_vblank_event(
> >> -		struct drm_device *dev, uint64_t user_data)
> >> +		struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
> >
> > Nit: Please also drop the dev argument, we have crtc->dev easily
> > available. That fits better into my long-term goal of getting rid of the
> > (dev, pipe) pairs everywhere in the vblank code and fully switching over
> > to drm_crtc *.
> 
> As 'dev' isn't used anyways, this seems like a fine plan.
> 
> >> +	switch (e->event.base.type) {
> >> +	case DRM_EVENT_VBLANK:
> >> +	case DRM_EVENT_FLIP_COMPLETE:
> >> +		if (seq)
> >> +			e->event.vbl.sequence = (u32) seq;
> >> +		if (now) {
> >> +			e->event.vbl.tv_sec = now->tv_sec;
> >> +			e->event.vbl.tv_usec = now->tv_nsec / 1000;
> >> +		}
> >> +		break;
> >> +	}
> >
> > Not sure why this change? Also prep for the new, presumably extended
> > events? Seems at least slightly inconsistent with other paths, where we
> > still unconditionally fill it in.
> 
> Yes, this prepares for the new events to make that patch smaller. The
> places where the data are still unconditionally assigned should know
> that the event in the struct is either a VBLANK or FLIP_COMPLETE.

Yeah, I realized that after reading the next patch carefully.

> >> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> >
> > This'll oops on ums drivers since kms isn't set up.
> 
> How about this fix?
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 857b7cf011e1..e39b2bd074e4 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  				  union drm_wait_vblank *vblwait,
>  				  struct drm_file *file_priv)
>  {
> -	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
>  	struct timespec now;
> @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	e->event.base.type = DRM_EVENT_VBLANK;
>  	e->event.base.length = sizeof(e->event.vbl);
>  	e->event.vbl.user_data = vblwait->request.signal;
> -	e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
> +	e->event.vbl.crtc_id = 0;
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +		if (crtc)
> +			e->event.vbl.crtc_id = crtc->base.id;
> +	}
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);

lgtm.

> > Or maybe I shouldn't have told you this and seized this opportunity to
> > break all the old drivers :-)
> 
> You now know my evil plan :-)

:-)

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web