[PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]

2017-10-12 Thread Keith Packard 
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.

v2:
 * Remove 'dev' argument from create_vblank_event

It wasn't being used anyways, and if we need it in the future,
we can always get it from crtc->dev.

 * Check for MODESETTING before looking for crtc in queue_vblank_event

UMS drivers will oops if we try to get a crtc, so make sure
we're modesetting before we try to find a crtc_id to fill into
the event.

Signed-off-by: Keith Packard 
---
 drivers/gpu/drm/drm_atomic.c |  7 +++---
 drivers/gpu/drm/drm_plane.c  |  2 +-
 drivers/gpu/drm/drm_vblank.c | 41 +---
 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, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 268969fecee7..1e2021bed265 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1815,7 +1815,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_crtc *crtc, uint64_t user_data)
 {
struct drm_pending_vblank_event *e = NULL;
 
@@ -1825,7 +1825,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;
 }
@@ -2079,7 +2080,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(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 6af02c7b5da3..574fd1475214 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1039,7 +1039,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 47460589d4cd..d250606cc771 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -804,20 +804,23 @@ static void send_vblank_event(struct drm_device *dev,
struct drm_pending_vblank_event *e,
u64 seq, ktime_t now)
 {
-   struct timespec64 tv = ktime_to_timespec64(now);
-
-   e->event.sequence = seq;
-   /*
-* e->event is a user space structure, with hardcoded unsigned
-* 32-bit seconds/microseconds. This is safe as we always use
-* monotonic timestamps since linux-4.15
-*/
-   e->event.tv_sec = tv.tv_sec;
-   e->event.tv_usec = tv.tv_nsec / 1000;
-
-   trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
-e->event.sequence);
+   struct timespec64 tv;
 
+   switch (e->event.base.type) {
+   case DRM_EVENT_VBLANK:
+   case DRM_EVENT_FLIP_COMPLETE:
+   tv = ktime_to_timespec64(now);
+   e->event.vbl.sequence = seq;
+   /*
+* e->event is a user space structure, with hardcoded unsigned
+* 32-bit seconds/microseconds. This is safe as we always use
+* monotonic timestamps since linux-4.15
+*/
+   e->event.vbl.tv_sec = tv.tv_sec;
+   e->event.vbl.tv_usec = tv.tv_nsec / 1000;
+   break;
+   }
+   trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
 }
 
@@ -869,7 +872,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,

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

2017-10-11 Thread Sean Paul 
On Tue, Oct 10, 2017 at 05:45:13PM -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.
> 
> v2:
>  * Remove 'dev' argument from create_vblank_event
> 
>   It wasn't being used anyways, and if we need it in the future,
>   we can always get it from crtc->dev.
> 
>  * Check for MODESETTING before looking for crtc in queue_vblank_event
> 
>   UMS drivers will oops if we try to get a crtc, so make sure
>   we're modesetting before we try to find a crtc_id to fill into
>   the event.
> 
> Signed-off-by: Keith Packard 

Reviewed-by: Sean Paul 
> ---
>  drivers/gpu/drm/drm_atomic.c |  7 ---
>  drivers/gpu/drm/drm_plane.c  |  2 +-
>  drivers/gpu/drm/drm_vblank.c | 30 ++
>  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, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 366c56fe5f58..08ff1b023f7b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1815,7 +1815,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_crtc *crtc, uint64_t user_data)
>  {
>   struct drm_pending_vblank_event *e = NULL;
>  
> @@ -1825,7 +1825,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;
>  }
> @@ -2079,7 +2080,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(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 72cba9805edc..192071209baa 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1039,7 +1039,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 8c3f2fdd821a..7d2674e0362a 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -823,14 +823,16 @@ static void send_vblank_event(struct drm_device *dev,
>  {
>   struct timeval tv;
>  
> - tv = ktime_to_timeval(now);
> - e->event.sequence = seq;
> - e->event.tv_sec = tv.tv_sec;
> - e->event.tv_usec = tv.tv_usec;
> -
> - 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:
> + tv = ktime_to_timeval(now);
> + e->event.vbl.sequence = seq;
> + e->event.vbl.tv_sec = tv.tv_sec;
> + e->event.vbl.tv_usec = tv.tv_usec;
> + break;
> + }
> + trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>   drm_send_event_locked(dev, &e->base);
>  }
>  
> @@ -882,7 +884,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);
> @@ -913,7 +914,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);
> @@ -1347,8 +13

[PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]

2017-10-10 Thread Keith Packard 
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.

v2:
 * Remove 'dev' argument from create_vblank_event

It wasn't being used anyways, and if we need it in the future,
we can always get it from crtc->dev.

 * Check for MODESETTING before looking for crtc in queue_vblank_event

UMS drivers will oops if we try to get a crtc, so make sure
we're modesetting before we try to find a crtc_id to fill into
the event.

Signed-off-by: Keith Packard 
---
 drivers/gpu/drm/drm_atomic.c |  7 ---
 drivers/gpu/drm/drm_plane.c  |  2 +-
 drivers/gpu/drm/drm_vblank.c | 30 ++
 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, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 366c56fe5f58..08ff1b023f7b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1815,7 +1815,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_crtc *crtc, uint64_t user_data)
 {
struct drm_pending_vblank_event *e = NULL;
 
@@ -1825,7 +1825,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;
 }
@@ -2079,7 +2080,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(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 72cba9805edc..192071209baa 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1039,7 +1039,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 8c3f2fdd821a..7d2674e0362a 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -823,14 +823,16 @@ static void send_vblank_event(struct drm_device *dev,
 {
struct timeval tv;
 
-   tv = ktime_to_timeval(now);
-   e->event.sequence = seq;
-   e->event.tv_sec = tv.tv_sec;
-   e->event.tv_usec = tv.tv_usec;
-
-   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:
+   tv = ktime_to_timeval(now);
+   e->event.vbl.sequence = seq;
+   e->event.vbl.tv_sec = tv.tv_sec;
+   e->event.vbl.tv_usec = tv.tv_usec;
+   break;
+   }
+   trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
 }
 
@@ -882,7 +884,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);
@@ -913,7 +914,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);
@@ -1347,8 +1347,14 @@ 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.use

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

2017-08-02 Thread Daniel Vetter 
On Mon, Jul 31, 2017 at 10:03:05PM -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.
> 
> v2:
>  * Remove 'dev' argument from create_vblank_event
> 
>   It wasn't being used anyways, and if we need it in the future,
>   we can always get it from crtc->dev.
> 
>  * Check for MODESETTING before looking for crtc in queue_vblank_event
> 
>   UMS drivers will oops if we try to get a crtc, so make sure
>   we're modesetting before we try to find a crtc_id to fill into
>   the event.
> 
> Signed-off-by: Keith Packard 
> ---
>  drivers/gpu/drm/drm_atomic.c |  7 ---
>  drivers/gpu/drm/drm_plane.c  |  2 +-
>  drivers/gpu/drm/drm_vblank.c | 30 ++
>  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, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..272b83ea9369 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_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(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;

You missed assigning crtc_id here. With that fixes:

Reviewed-by: Daniel Vetter 

Might also be good to check igt coverage for the various corner-cases
here.

> + 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 346601ad698d..7e7119a5ada3 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -804,14 +804,16 @@ static void send_vblank_event(struct drm_device *dev,
>  {
>   struct timeval tv;
>  
> - tv = ktime_to_timeval(now);
> - e->event.sequence = seq;
> - e->event.tv_sec = tv.tv_sec;
> - e->event.tv_usec = tv.tv_usec;
> -
> - 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:
> + tv = ktime_to_timeval(now);
> + e->event.vbl.sequence = seq;
> + e->event.vbl.tv_sec = tv.tv_sec;
> + e->event.vbl.tv_usec = tv.tv_usec;
> + break;
> + }
> + trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>   drm_send_event_locked(dev, &e->base);
>  }
>  
> @@ -863,7 +865,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);
> @@ -894,7 +895,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>   now = get_drm_timestamp();
>   }
>   e->pipe = pipe;
> - e->event.crtc

[PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]

2017-07-31 Thread Keith Packard 
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.

v2:
 * Remove 'dev' argument from create_vblank_event

It wasn't being used anyways, and if we need it in the future,
we can always get it from crtc->dev.

 * Check for MODESETTING before looking for crtc in queue_vblank_event

UMS drivers will oops if we try to get a crtc, so make sure
we're modesetting before we try to find a crtc_id to fill into
the event.

Signed-off-by: Keith Packard 
---
 drivers/gpu/drm/drm_atomic.c |  7 ---
 drivers/gpu/drm/drm_plane.c  |  2 +-
 drivers/gpu/drm/drm_vblank.c | 30 ++
 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, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c0f336d23f9c..272b83ea9369 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_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(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 346601ad698d..7e7119a5ada3 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -804,14 +804,16 @@ static void send_vblank_event(struct drm_device *dev,
 {
struct timeval tv;
 
-   tv = ktime_to_timeval(now);
-   e->event.sequence = seq;
-   e->event.tv_sec = tv.tv_sec;
-   e->event.tv_usec = tv.tv_usec;
-
-   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:
+   tv = ktime_to_timeval(now);
+   e->event.vbl.sequence = seq;
+   e->event.vbl.tv_sec = tv.tv_sec;
+   e->event.vbl.tv_usec = tv.tv_usec;
+   break;
+   }
+   trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
 }
 
@@ -863,7 +865,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);
@@ -894,7 +895,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);
@@ -1354,8 +1354,14 @@ 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_

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

2017-07-07 Thread Daniel Vetter 
On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote:
> Daniel Vetter  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

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

2017-07-06 Thread Keith Packard 
Daniel Vetter  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


signature.asc
Description: PGP signature

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

2017-07-06 Thread Daniel Vetter 
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 

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;

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

2017-07-05 Thread Keith Packard 
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 
---
 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,